[GUI] Better event filtering system #801

archibate · 2020-04-16T14:33:51Z

Related issue = #... (if any)

We used to support filter by only type.
Now we also support filter by key, or by (type, key) combination.

by key:

import taichi as ti
gui = ti.GUI('Hello')
while not gui.get_event(ti.GUI.ESCAPE):  # has ESC event
    gui.show()

by key + type:

import taichi as ti
gui = ti.GUI('Hello')
while not gui.get_event(ti.GUI.RELEASE, ti.GUI.ESCAPE):  # has REL or ESC event
    gui.show()

import taichi as ti
gui = ti.GUI('Hello')
while not gui.get_event((ti.GUI.RELEASE, ti.GUI.ESCAPE)):  # has REL and ESC event
    gui.show()

by comb:

import taichi as ti
gui = ti.GUI('Hello')
filt = [(ti.GUI.PRESS, ti.GUI.ESCAPE), (ti.GUI.RELEASE, ti.GUI.SPACE)]
while not gui.get_event(*filt):  # PRESS and ESC  or  RELEASE and SPACE
    gui.show()

python/taichi/misc/gui.py

k-ye · 2020-04-18T07:50:23Z

python/taichi/misc/gui.py

@@ -123,30 +123,53 @@ def line(self, begin, end, radius, color):
        self.canvas.path_single(begin[0], begin[1], end[0], end[1], color,
                                radius)

-    def show(self, file=None):
+    def show(self, file_or_image=None):


how about having two args: file and image? Then users can both show it and save it as files

I would suggest them:

gui.set_image(image); gui.show(file).

gui.show(); ti.imwrite(image, file).

I prefer 2, since I think show(file) is just a backward-compat-deprecated API. With ti.imwrite, users can even save images without GUI, provides more flexibility.

Then i think this should just be image, because imwrite is already able to save to files? Again, if you think at the API layer, GUI is supposed to deal with one thing -- GUI. And we have separate APIs for file writings. WDYT?

First of all, this part does not belong to the event filtering system. Please revert.

Please understand that having too many things in a PR makes reviewing really hard since the discussions cannot be focused on the real thing. This also increases the delay in merging your contribution since there are too many to discuss, and will result in nasty merge conflicts.

My thoughts on this specific issue: I agree that ti.GUI.show should not provide the option to write to disk. We might still need an API to get the image from the GUI or save the GUI image to disk, since sometimes people use drawing commands such as ti.GUI.circles to paint, and the painted result is only available from the GUI. Maybe we should do

gui.set_image to set a background image and allow people to paint on it

gui.show(image=None) to show the current canvas, or a plain 2D image (if image not none). Clear the temporary canvas after window content is updated.

gui.screenshot(filename=None) -> np.ndarray return the canvas. If filename is not None, use imwrite to save to disk. ti.imwrite(gui.screenshot(), 'X.png') sounds slightly verbose so I think a shortcut would be good.

The API design here should be put into a separate issue.

k-ye · 2020-04-18T07:52:01Z

python/taichi/misc/gui.py

                break
        return True

-    def get_key_event(self):
+    def get_cursor_pos(self):  # ABi


What's ABi?

get_cursor_pos is an alias of self.core.get_cursor_pos, which is in C++ scope, so it's an ABI.

hmm, feels a bit redundant, but sounds good

Sorry but I'm still confused about this: does ABi here stand for Application binary interface? Why is i lowercased here?

python/taichi/misc/gui.py

k-ye · 2020-04-18T07:57:51Z

Sorry for the late response..

archibate · 2020-04-18T08:58:15Z

Btw, do we have some deprecated function need to be marked as deprecated? Like ti.x86_64 -> ti.x64, ti.global_var -> ti.var? I found this: https://python-deprecated.readthedocs.io/en/latest.

k-ye

Sorry for my late response recently... I'm following this tutorial http://www.realtimerendering.com/raytracing/Ray%20Tracing%20in%20a%20Weekend.pdf and writing a toy ray tracer on Metal these days... I believe it also offers me insights of how to use/improve Taichi (For example, I just found out that Metal doesn't support recursion function calls!)

k-ye · 2020-04-19T03:28:00Z

python/taichi/misc/gui.py

@@ -123,30 +123,53 @@ def line(self, begin, end, radius, color):
        self.canvas.path_single(begin[0], begin[1], end[0], end[1], color,
                                radius)

-    def show(self, file=None):
+    def show(self, file_or_image=None):


Then i think this should just be image, because imwrite is already able to save to files? Again, if you think at the API layer, GUI is supposed to deal with one thing -- GUI. And we have separate APIs for file writings. WDYT?

python/taichi/misc/gui.py

archibate · 2020-04-19T13:03:42Z

Ok, now the API becomes:
peek: if has then return e; else return None.
get: if has then pop and return e; else return None.
wait: block until has.
wait_get: if has then pop and return e; else block until has, then pop and return e.

archibate · 2020-04-19T13:07:02Z

Then i think this should just be image, because imwrite is already able to save to files? Again, if you think at the API layer, GUI is supposed to deal with one thing -- GUI. And we have separate APIs for file writings. WDYT?

our imread/imwrite seems to has some bug now ([misc] imread bug, snowing image #802).
we need to ensure backward compatibility and make old examples work.
It's very convenient to screenshot in one line.
It's not a big deal to do an isinstance.

k-ye · 2020-04-20T09:45:09Z

I'll just leave this to @yuanming-hu to decide the API changes. In general, I think there are a few things.

This PR is, once again, getting too big and losing focus from what it intends to be. I thought you were just going to enhance the filtering, but now we have to debate over peek/get/wait_get. And it's pulling in non GUI event things like gui.show and rgb_to_hex.
Again, while I think it's valuable to have both peek and get, this combination of event, events, {get_, is_} x {key_pressed, is_pressed, key_event} is just making things really confusing. Personally i think it would be better if we could first list out what APIs we want, then start implementing them.

archibate · 2020-04-20T10:49:56Z

Yes, it did get confusing... let's just merge the first commit in this PR?

yuanming-hu · 2020-04-20T15:44:19Z

Sorry I have meetings until early afternoon. I'll take care of this thread later today. Thanks for being patient and for the valuable discussions here.

archibate · 2020-04-20T16:24:01Z

'm following this tutorial http://www.realtimerendering.com/raytracing/Ray%20Tracing%20in%20a%20Weekend.pdf and writing a toy ray tracer on Metal these days...

Oh this tutorial is cool! Have fun!

(For example, I just found out that Metal doesn't support recursion function calls!)

Interesting, so does GLSL... Seems most modern shader langs don't support recursion for simplicity, how about CUDA? And that may explain why simple ray tracers usually use CPU... hopefully taichi could change this role :)
How do you deal with this limitation then? Maybe portable in taichi's force-inlined function system too.

yuanming-hu

Hello? I thought I made a good PR description...

The description is indeed well-written. Good job! Sorry I've got too many things to do recently do my review might be delayed.

你没有阻止我那样做，甚至还对那些顺手牵羊的代码做了一些改进建议...

No need to be rude here. It should be the PR authors' (not reviewers') duty to prevent PRs from getting big. After all, reviewers cannot stop PR authors from adding unrelated commits. The only thing reviewers can do to keep development in order, is refusing to review/merge PRs with unrelated changes. I also highly recommend a re-reading of https://github.com/yuanming-hu/public_files/blob/master/graphics/taichi/google_review_comments.pdf

I'll continue my review after existing comments are addressed.

examples/image_io.py

python/taichi/misc/gui.py

yuanming-hu · 2020-04-20T22:01:17Z

python/taichi/misc/gui.py

                break
        return True

-    def get_key_event(self):
+    def get_cursor_pos(self):  # ABi


Sorry but I'm still confused about this: does ABi here stand for Application binary interface? Why is i lowercased here?

yuanming-hu · 2020-04-20T22:02:36Z

python/taichi/misc/gui.py

+            e = self._peek_key_event()
+        return e
+
+    def get_key_event(self):  # Unique APi


Again, if APi stands for application programming interfaces, why is i lowercased?

yuanming-hu · 2020-04-20T22:22:47Z

python/taichi/misc/gui.py

@@ -123,30 +123,53 @@ def line(self, begin, end, radius, color):
        self.canvas.path_single(begin[0], begin[1], end[0], end[1], color,
                                radius)

-    def show(self, file=None):
+    def show(self, file_or_image=None):


First of all, this part does not belong to the event filtering system. Please revert.

Please understand that having too many things in a PR makes reviewing really hard since the discussions cannot be focused on the real thing. This also increases the delay in merging your contribution since there are too many to discuss, and will result in nasty merge conflicts.

My thoughts on this specific issue: I agree that ti.GUI.show should not provide the option to write to disk. We might still need an API to get the image from the GUI or save the GUI image to disk, since sometimes people use drawing commands such as ti.GUI.circles to paint, and the painted result is only available from the GUI. Maybe we should do

gui.set_image to set a background image and allow people to paint on it

gui.show(image=None) to show the current canvas, or a plain 2D image (if image not none). Clear the temporary canvas after window content is updated.

gui.screenshot(filename=None) -> np.ndarray return the canvas. If filename is not None, use imwrite to save to disk. ti.imwrite(gui.screenshot(), 'X.png') sounds slightly verbose so I think a shortcut would be good.

The API design here should be put into a separate issue.

archibate · 2020-04-23T08:31:45Z

Fully reverted, only event filtering preserved!

k-ye

Thanks for taking the suggestions!

python/taichi/misc/gui.py

archibate · 2020-04-23T14:48:08Z

AppVeyor failed due to network reason again:

Build started
git clone -q https://github.com/taichi-dev/taichi.git C:\taichi
git fetch -q origin +refs/pull/801/merge:
git checkout -qf FETCH_HEAD
set TAICHI_REPO_DIR=C:\taichi
%PYTHON% %TAICHI_REPO_DIR%/misc/appveyor_filter.py || appveyor exit 0
cd C:\
curl --retry 10 --retry-delay 5 https://github.com/yuanming-hu/taichi_assets/releases/download/llvm8/taichi-llvm-8.0.1-msvc2017.zip -LO
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:--  0:00:34 --:--:--     0
curl: (52) Empty reply from server
Command exited with code 52

k-ye

LGTM (with one nit)

Personally I think the OR semantics is more intuitive than AND. There could be several open-ended questions regarding AND:

1.Since currently we don't check the tuple size, in theory a user could pass in an AND tuple with a size larger than 2. E.g. (TI.RELEASE, TI.ESCAPE, TI.SPACE). Should we disallow this? Or should we consider supporting it in the future?
2. If we do, then are (TI.RELEASE, TI.ESCAPE, TI.SPACE) and (TI.RELEASE, SPACE, TI.ESCAPE) equivalent, or even (TI.SPACE, TI.RELEASE, TI.ESCAPE)?

That said, I think this PR is good enough.

k-ye · 2020-04-26T14:19:18Z

python/taichi/misc/gui.py

+        return False
+
+    def get_events(self, *filter):
+        filter = filter and GUI.EventFilter(*filter) or None


I know I suggested filter or None, but in this case, i think the idiomatic way is GUI.EventFilter(*filter) if filter else None.

archibate · 2020-04-26T14:52:45Z

1.Since currently we don't check the tuple size, in theory a user could pass in an AND tuple with a size larger than 2. E.g. (TI.RELEASE, TI.ESCAPE, TI.SPACE). Should we disallow this? Or should we consider supporting it in the future?
2. If we do, then are (TI.RELEASE, TI.ESCAPE, TI.SPACE) and (TI.RELEASE, SPACE, TI.ESCAPE) equivalent, or even (TI.SPACE, TI.RELEASE, TI.ESCAPE)?

Thank for purposing this, but we won't support this. Why can a event be both ESC and Space? There is only one key pre-event, no multiple keys, if they want, they use gui.is_pressed(ESCAPE, SPACE). Let's just assert len(the_tuple) == 2 now.

archibate · 2020-04-26T16:37:50Z

Postponed by #839.

archibate · 2020-04-29T09:19:25Z

Added doc!

docs/gui.rst

archibate requested review from yuanming-hu and k-ye April 16, 2020 14:34

archibate changed the title ~~[GUI] better GUI filter system (gui.get_event)~~ [GUI] better GUI event filtering system Apr 16, 2020

This comment has been minimized.

Sign in to view

k-ye reviewed Apr 18, 2020

View reviewed changes

archibate requested a review from k-ye April 18, 2020 09:04

k-ye reviewed Apr 19, 2020

View reviewed changes

archibate requested a review from k-ye April 19, 2020 13:40

archibate changed the title ~~[GUI] better GUI event filtering system~~ [GUI] better event filtering system Apr 20, 2020

This comment has been minimized.

Sign in to view

yuanming-hu requested changes Apr 20, 2020

View reviewed changes

archibate requested a review from yuanming-hu April 21, 2020 04:12

archibate added 9 commits April 22, 2020 11:17

[GUI] better GUI filter system (gui.get_event)

8dc57ce

[skip ci] GUI.EventFilter

22f05d7

[skip ci] set()

b577d22

[skip ci] rename

4d648e5

[skip ci] revert

ec53b51

fix usage & add yield

0ee5fde

[skip ci] bette example

f70050a

[skip ci] refactor

164b424

[skip ci] better rgb_to_hex

63a74ee

archibate added 3 commits April 23, 2020 16:21

merge master

9ae212d

[skip ci] revert comp

47d3614

[skip ci] arrange

9944ab9

[skip ci] enforce code format

e2da020

k-ye reviewed Apr 23, 2020

View reviewed changes

python/taichi/misc/gui.py Outdated Show resolved Hide resolved

python/taichi/misc/gui.py Show resolved Hide resolved

python/taichi/misc/gui.py Outdated Show resolved Hide resolved

archibate added 2 commits April 23, 2020 22:39

[skip ci] apply reviews

80ac224

Merge branch 'gui' of github.com:archibate/taichi into gui

738f60f

archibate requested a review from k-ye April 23, 2020 14:40

yuanming-hu changed the title ~~[GUI] better event filtering system~~ [GUI] Better event filtering system Apr 23, 2020

archibate added 2 commits April 25, 2020 14:08

Merge branch 'master' into gui

9434684

better OR logic

afe64ef

archibate self-assigned this Apr 25, 2020

archibate requested review from k-ye and removed request for k-ye April 26, 2020 04:53

k-ye approved these changes Apr 26, 2020

View reviewed changes

assert len(combination entry) == 2

2efeba3

archibate requested review from yuanming-hu and removed request for yuanming-hu April 27, 2020 16:31

archibate added 2 commits April 29, 2020 16:00

[skip ci] fix doc

fce41a7

[skip ci] update doc for gui

0b56be0

archibate requested a review from k-ye April 29, 2020 09:18

Merge branch 'gui' of github.com:archibate/taichi into gui

4cbb163

archibate commented Apr 29, 2020

View reviewed changes

docs/gui.rst Outdated Show resolved Hide resolved

[skip ci] Update docs/gui.rst

6a5a44d

archibate merged commit 6751bd5 into taichi-dev:master Apr 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GUI] Better event filtering system #801

[GUI] Better event filtering system #801

archibate commented Apr 16, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

k-ye Apr 18, 2020

archibate Apr 18, 2020 •

edited

Loading

k-ye Apr 19, 2020

yuanming-hu Apr 20, 2020

k-ye Apr 18, 2020

archibate Apr 18, 2020

k-ye Apr 18, 2020

yuanming-hu Apr 20, 2020

k-ye commented Apr 18, 2020

archibate commented Apr 18, 2020 •

edited

Loading

k-ye left a comment •

edited

Loading

k-ye Apr 19, 2020

archibate commented Apr 19, 2020 •

edited

Loading

archibate commented Apr 19, 2020

k-ye commented Apr 20, 2020

archibate commented Apr 20, 2020

This comment has been minimized.

yuanming-hu commented Apr 20, 2020

archibate commented Apr 20, 2020 •

edited

Loading

yuanming-hu left a comment

yuanming-hu Apr 20, 2020

yuanming-hu Apr 20, 2020

archibate Apr 21, 2020

yuanming-hu Apr 20, 2020

archibate commented Apr 23, 2020

k-ye left a comment

archibate commented Apr 23, 2020

k-ye left a comment

k-ye Apr 26, 2020

archibate commented Apr 26, 2020

archibate commented Apr 26, 2020

archibate commented Apr 29, 2020 •

edited

Loading

[GUI] Better event filtering system #801

[GUI] Better event filtering system #801

Conversation

archibate commented Apr 16, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

archibate Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-ye commented Apr 18, 2020

archibate commented Apr 18, 2020 • edited Loading

k-ye left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archibate commented Apr 19, 2020 • edited Loading

archibate commented Apr 19, 2020

k-ye commented Apr 20, 2020

archibate commented Apr 20, 2020

This comment has been minimized.

yuanming-hu commented Apr 20, 2020

archibate commented Apr 20, 2020 • edited Loading

yuanming-hu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archibate commented Apr 23, 2020

k-ye left a comment

Choose a reason for hiding this comment

archibate commented Apr 23, 2020

k-ye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archibate commented Apr 26, 2020

archibate commented Apr 26, 2020

archibate commented Apr 29, 2020 • edited Loading

archibate commented Apr 16, 2020 •

edited

Loading

archibate Apr 18, 2020 •

edited

Loading

archibate commented Apr 18, 2020 •

edited

Loading

k-ye left a comment •

edited

Loading

archibate commented Apr 19, 2020 •

edited

Loading

archibate commented Apr 20, 2020 •

edited

Loading

archibate commented Apr 29, 2020 •

edited

Loading