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

Support GUI key/mouse events on Mac OS X #478

Merged
merged 1 commit into from
Feb 16, 2020
Merged

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Feb 16, 2020

Follow up of PR #459 by @archibate

I think there are some opportunities for refactoring more:

  1. All the key string constants (e.g. "Shift") can be defined in a single place and shared by all platforms.
  2. MouseEvent should probably include a button field to distinguish left, middle or right.
    struct MouseEvent {
  3. For mouse events, we can do the push_back here:
    void mouse_event(MouseEvent e) {
  4. Actually, I think key_events is a little bit abused for holding mouse events. Maybe have ti.has_input_event() and ti.get_input_event()? The item returned by ti.get_input_event() has a type to tell if it's a mouse or a keyboard event (or any other input device). But i guess this can be deferred because it's not that trivial and the current GUI works OK enough. @yuanming-hu @archibate Thoughts?

@archibate
Copy link
Collaborator

archibate commented Feb 16, 2020

1. All the key string constants (e.g. `"Shift"`) can be defined in a single place and shared by all platforms.

Currently they are at

SHIFT = 'Shift'

We want it move into gui.h and then exported into gui.py.
But this takes a lot of works, and benefits a little - keyboards are unlikely to change, we don't have to keep a table of all key constants. I personally think it doesn't worth it.

@archibate
Copy link
Collaborator

2\. `MouseEvent` should probably include a button field to distinguish left, middle or right.

Actually we already have that, eg. e.type == ti.GUI.LMB.

@k-ye
Copy link
Member Author

k-ye commented Feb 16, 2020

Sorry about the confusion. I don't mean to export them to python. Instead, just create something like taichi/gui/constants.h that contains these string constants, then all these platform-specific GUI impls can include that header.

Actually we already have that, eg. e.type == ti.GUI.LMB.

I mean another enum class (e.g. enum class MouseButton {left, right, middle}; inside MouseEvent :-)

@archibate
Copy link
Collaborator

archibate commented Feb 16, 2020

But how to keep gui.py the same with gui/constants.h without export? Maybe a code generator will help.

@archibate
Copy link
Collaborator

archibate commented Feb 16, 2020

Actually, I think key_events is a little bit abused for holding mouse events. Maybe have ti.has_input_event() and ti.get_input_event()?

My opinion, mouse_event -> widget_event, key_event -> input_event.
Currently I see mouse_event doing widget message transferring works by detecting mouse position. It can be modified to support key message passing too!
While key_event doesn't care widget border, can deal both keyboard and mouse input.
I think they are two things. BTW, I see Slider widget there, but not used.

@k-ye
Copy link
Member Author

k-ye commented Feb 16, 2020

But how to keep gui.py the same with gui/constants.h without export? Maybe a code generator will help.

I kind of feel like that's a bit of an overkill as well... My original intention was really just to share these constants among taichi/gui...

But if we want to do this in a really fancy way, maybe have our own enums (e.g. enum class KeyCode { SHIFT, ESCAPE, ...} for all the keys we support, and define a std::string key_code_name(KeyCode)... Then we can export the enum and the key_code_name() to python. You can take a look at the Arch enum :)

enum class Arch {

@archibate
Copy link
Collaborator

archibate commented Feb 16, 2020

Great! Just enum class KeyCode can distinguish all keys supported, key_code_name really necessary?
Wait, you mean key_code_name(KeyCode::SHIFT) = "SHIFT"?

#define DEF_KEY_CODE(x) case KeyCode::x: return #x;
...
std::string key_code_name(KeyCode c)
{
  switch (c) {
  DEF_KEY_CODE(SHIFT)
  ...
  }
  return "UnknownKey";
}

@k-ye
Copy link
Member Author

k-ye commented Feb 16, 2020

My opinion, mouse_event -> widget_event, key_event -> input_event.

I feel like Widgets event shouldn't be mixed with mouse/keyboard events. Widgets are part of the GUI hierarchy, and should be the responder/handler of the input device events. The mouse_events() defined on those widget should probably be named onMouseEvent()...

enum class KeyCode can distinguish all keys supported, key_code_name really necessary?

The later can be useful for debugging purpose. But yeah, i think the enum is the essential thing here...

@archibate
Copy link
Collaborator

For mouse events, we can do the push_back here:

The reason why I mix mouse into key_events was, if we have two queue or two class with same interface, then it is hard to get data from it. eg. e.button for MouseEvent and e.key for KeyEvent.
Since e.button has no conflict with e.key - they're all enum values.
Also, I noticed that XKeyEvent has member named x and y... represents the mouse position when key event happened. So I think it's no problem to combine them together.

@k-ye
Copy link
Member Author

k-ye commented Feb 16, 2020

Yeah, as I said, the GUI works pretty well. I don't think any of these are immediate problems...

@yuanming-hu yuanming-hu self-requested a review February 16, 2020 15:25
@yuanming-hu yuanming-hu merged commit 6355480 into taichi-dev:master Feb 16, 2020
@yuanming-hu
Copy link
Member

yuanming-hu commented Feb 16, 2020

Thanks for the PR and discussions! I'll ship the new interactive features in v0.5.1 by tonight. I believe having a GUI will make people have much more fun with Taichi, yet forgive me that I won't have too much time to think about it right now, since I've got enough things to do (i.e. make sparse computation codegen fully functional and optimized, which I initially promised to finish by end of Jan...). @archibate @k-ye Please feel free to make your own design decisions regarding the GUI. I will probably champion your decisions. About the widgets: #484

@k-ye k-ye deleted the mackey branch February 17, 2020 01:05
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