[GUI] Support saving image without GUI showing a window #1817

thinking-tower · 2020-09-01T13:04:15Z

Fixes #1616. cc @archibate @yuanming-hu.

Description

As mentioned in #1616 by @ehannigan, saving the screenshot of a GUI without an actual window will throw

"RuntimeError: [x11.cpp:create_window@139] Taichi fails to create a window. This is probably due to the lack of an X11 GUI environment. If you are using ssh, try ssh -XY."

It is possible to do this with matplotlib however it would be better to have a native solution. This PR provides the functionality to save an image without showing a GUI as well as correcting instances of idendity.

Concerns

Since I'm putting a if statement around self.core.update(), is it a concern that the internal GUI class has a different state from ti.GUI class?

Original version is shown here:

taichi/python/taichi/misc/gui.py

Lines 358 to 363 in 6052a54

    
           def show(self, file=None): 
        
               self.core.update() 
        
               if file: 
        
                   self.core.screenshot(file) 
        
               self.frame += 1 
        
               self.clear()

I made a test_gui.py but there is also another test_gui.py in misc/.
It could be better to recommend that if show_GUI is False, that users use gui.core.screenshot("filename.jpg") instead of altering the show function.

- Add parameter to indicate show GUI or not. - Move GUI dependent code in constructor into a separate function. - Add a if statement around updating GUI.

yuanming-hu

Thank you so much! This mostly looks good to me. I only left a few minor comments.

python/taichi/misc/gui.py

taichi/gui/gui.h

tests/python/test_gui.py

- Use ti.imread instead of Pillow. - Change spelling of initialise to initialize. - Use show_gui instead of show_GUI.

thinking-tower · 2020-09-02T03:42:28Z

Hey @yuanming-hu, how do I fix this warning?

Here's the relevant snippet:

pixels = ti.field(dtype=ti.u8, shape=(n, n, 3))

@ti.kernel
def paint(c: dtype):
    for i, j, k in pixels:
        pixels[i, j, k] = c

for i in range(255):
    paint(i)

archibate · 2020-09-02T04:38:16Z

python/taichi/misc/gui.py

+                 show_gui=True,
+                 res=512,


Suggested change

show_gui=True,

res=512,

res=512,

show_gui=True,

Some examples wil break by the argument order.

taichi/gui/gui.h

archibate · 2020-09-02T04:40:25Z

taichi/gui/gui.h

    while (last_frame_interval.size() > 30) {
      last_frame_interval.erase(last_frame_interval.begin());
    }
    auto real_fps = last_frame_interval.size() /
                    (std::accumulate(last_frame_interval.begin(),
                                     last_frame_interval.end(), 0.0_f));
+    if (show_gui) {


In fact, we can ignore the FPS-saving sleep procedure when we just want to save images.

@archibate actually should I also move taichi::Time::wait_until(last_frame_time + frame_delta_limit) to under the if (show_gui)?

Yes, I think actually we can move all the codes to update_gui()? Or simply add if (!show_gui) return; to the first line of update()?

archibate · 2020-09-02T04:41:07Z

tests/python/test_gui.py

+        for i, j, k in pixels:
+            pixels[i, j, k] = c
+
+    gui = ti.GUI("Test", show_gui=False, res=(n, n))


Suggested change

gui = ti.GUI("Test", show_gui=False, res=(n, n))

gui = ti.GUI("Test", (n, n), show_gui=False)

I think it would be a good idea to keep the named argument for res in case of constructor order changing?

For example, examples/stable_fluid.py is using ti.GUI('Stable Fluid', (res, res)), so it's already broken by this change. I believe many end-users are using res as positional argument too.
No matter if positional argument is a good design or not, we always want to enforce backward compatibility as much as possible.

Here, I've moved show_gui to the last argument in the constructor now so that the original order is preserved:

def __init__(self, name='Taichi', res=512, background_color=0x0, show_gui=True):

archibate

Also add if (show_gui) guard to cocoa.cpp?

thinking-tower · 2020-09-02T05:37:08Z

Also add if (show_gui) guard to cocoa.cpp?

Since all the OS X window resources/functions are called in process_event, set_title, redraw, redraw_widgets (which have been wrapped in update_gui and I've added

if (show_gui) {
    update_gui();
}

, it seems to be okay for now.

I added if statements for win32.cpp and x11.cpp because they are releasing resources in the destructor unlike in cocoa.cpp.

- Count fps if show_gui is true. - Move show_gui to last argument to avoid breaking old examples.

codecov · 2020-09-02T06:22:38Z

Codecov Report

Merging #1817 into master will increase coverage by 0.74%.
The diff coverage is 17.83%.

@@            Coverage Diff             @@
##           master    #1817      +/-   ##
==========================================
+ Coverage   43.35%   44.09%   +0.74%     
==========================================
  Files          43       44       +1     
  Lines        5995     6064      +69     
  Branches     1040     1084      +44     
==========================================
+ Hits         2599     2674      +75     
+ Misses       3246     3221      -25     
- Partials      150      169      +19

Impacted Files	Coverage Δ
python/taichi/core/__init__.py	`0.00% <ø> (ø)`
python/taichi/core/util.py	`0.37% <0.00%> (+0.08%)`	⬆️
python/taichi/diagnose.py	`0.00% <0.00%> (ø)`
python/taichi/lang/__init__.py	`41.94% <0.00%> (-11.67%)`	⬇️
python/taichi/lang/shell.py	`0.00% <0.00%> (ø)`
python/taichi/lang/util.py	`31.73% <0.00%> (+0.37%)`	⬆️
python/taichi/tools/np2ply.py	`0.00% <0.00%> (ø)`
python/taichi/main.py	`23.36% <4.54%> (-0.12%)`	⬇️
python/taichi/misc/gui.py	`8.28% <5.26%> (+8.28%)`	⬆️
python/taichi/lang/matrix.py	`68.87% <11.11%> (+3.84%)`	⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6052a54...c371e51. Read the comment docs.

yuanming-hu

Awesome! LGTM now. Just a final nit. I can merge if that suggestion is adopted and CI passes.

tests/python/test_gui.py

thinking-tower and others added 4 commits September 1, 2020 22:36

Change 'idendity' to 'identity'.

9d559b6

Changes:

115034f

- Add parameter to indicate show GUI or not. - Move GUI dependent code in constructor into a separate function. - Add a if statement around updating GUI.

Add test for saving image without window.

82bb015

[skip ci] enforce code format

b000a17

yuanming-hu reviewed Sep 1, 2020

View reviewed changes

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

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

taichi/gui/gui.h Show resolved Hide resolved

tests/python/test_gui.py Outdated Show resolved Hide resolved

thinking-tower added 4 commits September 2, 2020 10:43

Changes:

0d07386

- Use ti.imread instead of Pillow. - Change spelling of initialise to initialize. - Use show_gui instead of show_GUI.

Remove PIL dependency.

db8b290

Move show_gui back to C++. Execute GUI functions if show_gui is true.

a00f3a9

Free memory if show_gui is true.

622d6b9

thinking-tower requested a review from yuanming-hu September 2, 2020 03:57

archibate reviewed Sep 2, 2020

View reviewed changes

taichi/gui/gui.h Show resolved Hide resolved

archibate reviewed Sep 2, 2020

View reviewed changes

archibate suggested changes Sep 2, 2020

View reviewed changes

Changes:

253b6ed

- Count fps if show_gui is true. - Move show_gui to last argument to avoid breaking old examples.

thinking-tower requested a review from archibate September 2, 2020 05:47

yuanming-hu approved these changes Sep 3, 2020

View reviewed changes

tests/python/test_gui.py Outdated Show resolved Hide resolved

Update tests/python/test_gui.py

c371e51

yuanming-hu merged commit ba73639 into taichi-dev:master Sep 4, 2020

yuanming-hu mentioned this pull request Sep 5, 2020

[release] v0.6.32 #1841

Merged

archibate mentioned this pull request Sep 8, 2020

[error] [doc] Better error message when X11 environment failed to initialize #1856

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GUI] Support saving image without GUI showing a window #1817

[GUI] Support saving image without GUI showing a window #1817

thinking-tower commented Sep 1, 2020

yuanming-hu left a comment

thinking-tower commented Sep 2, 2020

archibate Sep 2, 2020

archibate Sep 2, 2020

thinking-tower Sep 4, 2020

archibate Sep 4, 2020

archibate Sep 2, 2020

thinking-tower Sep 2, 2020

archibate Sep 2, 2020

thinking-tower Sep 2, 2020

archibate left a comment

thinking-tower commented Sep 2, 2020 •

edited

Loading

codecov bot commented Sep 2, 2020 •

edited

Loading

yuanming-hu left a comment

	def show(self, file=None):
	self.core.update()
	if file:
	self.core.screenshot(file)
	self.frame += 1
	self.clear()

	gui = ti.GUI("Test", show_gui=False, res=(n, n))
	gui = ti.GUI("Test", (n, n), show_gui=False)

[GUI] Support saving image without GUI showing a window #1817

[GUI] Support saving image without GUI showing a window #1817

Conversation

thinking-tower commented Sep 1, 2020

Description

Concerns

yuanming-hu left a comment

Choose a reason for hiding this comment

thinking-tower commented Sep 2, 2020

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

Choose a reason for hiding this comment

archibate left a comment

Choose a reason for hiding this comment

thinking-tower commented Sep 2, 2020 • edited Loading

codecov bot commented Sep 2, 2020 • edited Loading

Codecov Report

yuanming-hu left a comment

Choose a reason for hiding this comment

thinking-tower commented Sep 2, 2020 •

edited

Loading

codecov bot commented Sep 2, 2020 •

edited

Loading