Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GUI.wait_key, GUI.get_key to python bindings. #459

Merged
merged 26 commits into from
Feb 14, 2020

Conversation

archibate
Copy link
Collaborator

@archibate archibate commented Feb 11, 2020

Old, user need to click on the 'X' button of window:

while True:
  render_gui_here()
  gui.show()

do_some_thing_other()

You get exit(1):

X connection to :0 broken (explicit kill or server shutdown).

and do_some_thing_other() won't be reached.

New, user can kill GUI with ESC or space key:

while not gui.core.get_key():
  render_gui_here()
  gui.show()

do_some_thing_other()

@archibate archibate changed the title Add GUI.wait_key GUI.get_key_pressed to python bindings. Add GUI.wait_key GUI.get_key to python bindings. Feb 11, 2020
@archibate archibate changed the title Add GUI.wait_key GUI.get_key to python bindings. Add GUI.wait_key, GUI.get_key to python bindings. Feb 11, 2020
@archibate
Copy link
Collaborator Author

how about gui.core.get_key -> gui.get_key?

@yuanming-hu
Copy link
Member

Thanks for the PR! Yes, missing a mechanism to kill the GUI from keyboards is a design pitty.

We should consider systematically redesigning the keyboard event handling. I feel like we should implement 3 APIs

  • key = gui.wait_key() simply block until a key is pressed. Return the key.
  • event = gui.get_key_event(), where event=(key, press_or_release flag, control_shift_alt flags)
  • bool = gui.is_pressed(key) query if a key is currently pressed.

This will allow Taichi users to create complex applications with keyboards, such as video games :-) What do you think?

@archibate
Copy link
Collaborator Author

Good idea!
Instead of just gui.is_pressed('xxx') I am using a set gui.key_pressed to keep track of every key pressed (in set) or not. For example, user can either use 'Shift_L' in gui.key_pressed or gui.is_pressed('Shift_L') to detect if left shift is pressed. How do you think?
Of course, 'Shift_L' in gui.key_pressed or 'Shift_R' in gui.key_pressed is too bad for user. But we can use gui.is_pressed('Shift') to simplify that.

@archibate
Copy link
Collaborator Author

Adding some aliases, eg. gui.is_pressed(GUI.LEFT) to test arrow left key.

@archibate
Copy link
Collaborator Author

gui.get_key_event will block user until a key event is happened.
To enable user do other things during waiting (eg. rendering video game :).
We introduced gui.has_key_event to detect if there is any key event, if not, user can continue their fps loop without being blocked.

@yuanming-hu
Copy link
Member

Awesome!

Instead of just gui.is_pressed('xxx') I am using a set gui.key_pressed to keep track of every key pressed (in set) or not. For example, user can either use 'Shift_L' in gui.key_pressed or gui.is_pressed('Shift_L') to detect if left shift is pressed. How do you think?

Let's make is_pressed the only API visible to users. This will make it easier for users to learn.

Of course, 'Shift_L' in gui.key_pressed or 'Shift_R' in gui.key_pressed is too bad for user. But we can use gui.is_pressed('Shift') to simplify that.

Yeah, I agree that people usually don't distinguish left/right shifts. Let's just use Shift.

gui.get_key_event will block user until a key event is happened.
To enable user do other things during waiting (eg. rendering video game :).
We introduced gui.has_key_event to detect if there is any key event, if not, user can continue their fps loop without being blocked.

Good idea! I guess people would do a main loop like

while True:
   while gui.has_key_event():
     event = gui.get_key_event()
     process(event)
   rendering, logic, etc. (user inputs during this period is added to the event queue)

@yuanming-hu
Copy link
Member

Before I merge this PR, could do

  • Fix compilation errors on Windows (Appveyor)? (Travis failure is due to the recently added Metal backend.)
  • Add a basic example (~10 lines of code) demonstrating the keyboard functionality. Potentially just use arrow keys to move a circle on screen (gui.Circle)?

@archibate
Copy link
Collaborator Author

Oh...why? I discarded all changes in gui/win32.cpp (except no-eol), but it still fails.

@archibate
Copy link
Collaborator Author

How can I make Travis fail? So that I can see the Details button of another fail. Well...it's ridiculous. But how can I see detailed message of the build when All checks have failed?

@archibate
Copy link
Collaborator Author

Okay...seems problem comes from the git >>>>> feature-gui signatures. After I resloved conflicts in gui.py, it begins to build normaly :)

@archibate
Copy link
Collaborator Author

ci.appveyor.com seems being blocked...

@archibate
Copy link
Collaborator Author

I can't see what's going on there, can you take a look of what's the error? Thanks!

@archibate
Copy link
Collaborator Author

Ok, accessible now.

@archibate
Copy link
Collaborator Author

Ok, I mistake format as a STL function std::format, but actually in taichi/util.h, names fmt::format.

@archibate
Copy link
Collaborator Author

Now event is a class Event, has attrs: e.key, e.type, e.modifier, e.pos.
e.pos means the mouse cursor position while key is pressed/released.
You can also call gui.get_cursor_pos() to do so.
If e.key is GUI.LMB, then this is a left mouse button event, e.pos is the click position.
If e.key is GUI.MOTION, then this is a mouse motion event, e.modifier also works.

@archibate
Copy link
Collaborator Author

Note: e.pos is expressed in canvas coor, i.e. (0,1)*(0,1).

@archibate
Copy link
Collaborator Author

We're almost done! The only thing left to do is add mouse support for win32. But before that I need wait to see if there is any build errors on appveyor.

@archibate
Copy link
Collaborator Author

Ok, both travis and apveyor build passed. You can merge this PR now :)

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!!

@yuanming-hu yuanming-hu merged commit 77aa05b into taichi-dev:master Feb 14, 2020
@yuanming-hu
Copy link
Member

The new keyboard system is fantastic! See

print("[Hint] Use WSAD/arrow keys to control gravity. Use left/right mouse bottons to attract/repel. (OS X not yet supported)")

@archibate
Copy link
Collaborator Author

Cool physics! It's so nice to see our keyboard system working properly :)

@archibate archibate deleted the feature-gui branch February 14, 2020 18:17
@archibate
Copy link
Collaborator Author

But lack of experience and environment of OS X, I'm not able to make keyboard support for OS X, sorry :) Maybe we should leave that development to ones who have a environment.

@k-ye
Copy link
Member

k-ye commented Feb 14, 2020

I can take a look at the Mac keyboard events :-) (BTW, it would be great if the commits can be squashed to keep the master branch a cleaner timeline...)

@yuanming-hu
Copy link
Member

I can take a look at the Mac keyboard events :-)

Great and thanks!

BTW, it would be great if the commits can be squashed to keep the master branch a cleaner timeline...

That's a good point. I think we should make simple rules for commits/commit messages/PRs and put it in https://taichi.readthedocs.io/en/latest/contributor_guide.html Let's move the discussions here: #466

@archibate
Copy link
Collaborator Author

Yeah, I see, commits titled fix gui/win32.cpp are so meanless, maybe we really should do something with them before merging into master commit log... Anyway, thank for your invitation for taichi-dev!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants