-
Notifications
You must be signed in to change notification settings - Fork 98
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
return correct sized tuples of all 0s not None if image plot resized too small #589
Conversation
I tested this locally by checking the example https://github.com/enthought/chaco/blob/174342ff70f78941f011656cc994a9c28463c547/examples/demo/image_plot_origin_and_orientation.py - and that example doesn't crash on this branch - whereas the example crashes on the main branch. But, I don't like the fact that I can squeeze the window horizontally and vertically into an absurd state. But, an working application in an absurd state is better than a crashed application i guess? |
I'm not sure why an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM. It'd be awesome if you could put together a regression test for this change, that would be awesome.
Let's wait for the devs who reported the issue to also get back to us.
@aaronayres35 @rahulporuri I tested and I don't see the bug anymore. Thank you for the fix. |
chaco/tests/test_image_plot.py
Outdated
def test_resize_to_zero(self): | ||
class TestResize(HasTraits): | ||
plot = Instance(Component) | ||
|
||
# at the exact size of (101, 101) for the component editor the | ||
# image is 0x0 on the screen and the failure was triggered | ||
traits_view = View( | ||
Group( | ||
UItem('plot', editor=ComponentEditor(size=(101, 101))), | ||
), | ||
resizable=True, | ||
) | ||
|
||
def _plot_default(self): | ||
pd = ArrayPlotData(image=face()) | ||
plot = Plot(pd) | ||
plot.img_plot('image') | ||
|
||
return plot | ||
|
||
test_obj = TestResize() | ||
tester = UITester() | ||
|
||
# should not fail | ||
with tester.create_ui(test_obj) as ui: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is a little ugly namely from the 101 magic number. I was unsure how to better write this test.
Without the fix this test fails, with it it passes.
I've attempted to write a regression test like how we discussed. It seems to do the trick although it is a little ugly. I added a comment to hopefully clear up the nature of the test / the magic number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
fixes #528 (I think)
With the change made in this PR I no longer see the crash described here: #528 (comment)
This PR simply updates
_calc_zoom_coords
to return what it claims to return:In this particular case if we have zoomed in so far that the image has either length or width 0, the the screen rect showing it is effectively non-existent, and there is no data to show in it.
Technically
((0, 0, 0, 0), (0, 0, 0, 0))
isn't really the exactly correct thing to return here (instead you could think that screen_rect should be a 0x0 rectangle at the point where the image should be on screen, not at x=0, y=0). However, there is actually nothing to draw so it effectively doesn't matter.The only place this method is called is by
_compute_cached_image
. If_compute_cached_image
gets((0, 0, 0, 0), (0, 0, 0, 0))
everything works out as expected as we end up withdata=data[0:0, 0:0]
makingdata.shape = (0, 0, 3)
and a screen_rect that is 0x0.Note: now when I slowly shrink the window once I hit the critical point rather than the example crashing, I see:
This is not new from the changes in this PR (was masked by the crash previously), but occurs because we end up with an
_axis_vector
of (0, 0) when we shrink that far