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

Minimal fix for list cropping problem #1274

Merged
merged 1 commit into from May 25, 2019

Conversation

tdaffin
Copy link
Contributor

@tdaffin tdaffin commented May 6, 2019

This fixes a problem where a scrollable canvas gets strangely cropped when inside another scrollable region.
This also applies to List, ListSelect and DropDownList.
To demonstrate the problem I've added a scrollable canvas and a List to the all_piston_window example on this commit:
tdaffin/conrod@b84ae50

I think that this will fix #955

@tdaffin
Copy link
Contributor Author

tdaffin commented May 8, 2019

In case it helps, this change essentially rolls back this PR: #1230
Perhaps this is a platform/window manager dependent problem?
I'm running on Ubuntu 18.04, but I'm not sure what platform @0e4ef622 was running on.

@0e4ef622
Copy link
Contributor

0e4ef622 commented May 9, 2019

I was running Arch Linux with i3wm.

@0e4ef622
Copy link
Contributor

0e4ef622 commented May 9, 2019

This might be related to PistonDevelopers/opengl_graphics#298

@0e4ef622
Copy link
Contributor

0e4ef622 commented May 9, 2019

And this reintroduces the exact bug that the code you're reverting fixes. I checked just to make sure.

@0e4ef622
Copy link
Contributor

0e4ef622 commented May 9, 2019

Hm that's strange, with your fix, the y coordinate for the scissor is backwards in my project but correct with your example, and vice versa without.

@tdaffin
Copy link
Contributor Author

tdaffin commented May 11, 2019

Can you post the code of your project @0e4ef622? If we could get your example and mine into the same layout in the same example it could be useful.

@0e4ef622
Copy link
Contributor

0e4ef622 commented May 11, 2019

Here's a gist. You'll need to add an arial.ttf to src/ or edit the path to the font file.

@tdaffin
Copy link
Contributor Author

tdaffin commented May 12, 2019

Thanks. I can reproduce your bug using this.
I'm working on making a single executable that exhibits both behaviors...

@tdaffin
Copy link
Contributor Author

tdaffin commented May 12, 2019

I've created a repository which uses the code from @0e4ef622's gist to show both behaviors.
https://github.com/tdaffin/conrod_bug
So far it looks like the main difference is that @0e4ef622's (in gl and gl_rend modules) uses opengl_graphics::GlGraphics whereas mine (in pist and pist_rend modules) uses piston_window::G2d.
I'm going to try to make the two versions use more of the same code while still showing the two different behaviors in an effort to home in on the real cause of the bug.

@tdaffin
Copy link
Contributor Author

tdaffin commented May 12, 2019

I made some more changes to the repro code to try and narrow down the bug some more.
I can't make both windows interactive at once (get a segfault), but I can get a screenshot of the two:
image

@0e4ef622
Copy link
Contributor

@tdaffin I think you should move this stuff to a new issue.

@tdaffin
Copy link
Contributor Author

tdaffin commented May 16, 2019

Yes, a new issue would probably be a good idea.
Not sure where to put it though so I'll just jot down my findings so far here...
At this point it looks like the problem is between two different ways to define coordinates for the scissor.
I'm not 100% sure as yet, but I think that most of piston/conrod and the back ends define coordinates relative to the upper left corner with x going left to right and y going down.
https://github.com/PistonDevelopers/graphics/blob/master/src/draw_state.rs looks like it is used that way, though it only mentions 'in screen space' by way of definition.

However, once the [u32; 4] arrives down inside of the gl_graphics backend in the bind_scissor function in https://github.com/PistonDevelopers/opengl_graphics/blob/master/src/draw_state.rs and is passed to gl::Scissor then it is required to be defined relative to the lower left corner with x going left to right and y going up. See https://www.khronos.org/opengl/wiki/Scissor_Test

... At least that is what I think is going on...

@tdaffin
Copy link
Contributor Author

tdaffin commented May 19, 2019

I've opened a new issue in opengl_graphics (PistonDevelopers/opengl_graphics#302) with a PR to fix it (PistonDevelopers/opengl_graphics#303) and posted a fixed version of the example that references both this PR and the one for conrod: https://github.com/tdaffin/conrod_bug/tree/fix_crop

I suppose it will be necessary to bump the version number on opengl_graphics once that PR is merged so that this PR can require the fixed version of opengl_graphics.

@tdaffin
Copy link
Contributor Author

tdaffin commented May 21, 2019

@0e4ef622 does the fix I made to piston2d-opengl_graphics make this this fix work for you?
It has been merged and published as version 0.60 and appears to make everything work in https://github.com/tdaffin/conrod_bug

@0e4ef622
Copy link
Contributor

Yep, the fix works. Nice job narrowing down the bug farther than I did when I first found it.

@tdaffin
Copy link
Contributor Author

tdaffin commented May 22, 2019

This may also fix #1166

@mitchmindtree
Copy link
Contributor

Thanks for looking into this @tdaffin and @0e4ef622!

What is the status of this PR? Would you still like to land this?

@tdaffin
Copy link
Contributor Author

tdaffin commented May 25, 2019

Yes, I think this should be merged with an increment to the conrod crate version.
That way those who are already using conrod 0.63 with piston2d-opengl_graphics 0.59 will just need to take 0.60 of piston2d-opengl_graphics to fix any problems when they update conrod.

@tdaffin
Copy link
Contributor Author

tdaffin commented May 25, 2019

FYI: A separate fix for #1166 will be required.

@mitchmindtree
Copy link
Contributor

OK thanks @tdaffin, I'm also interested in publishing a new version for the recent conrod_vulkano updates so I'll merge this and get to it 👍

@mitchmindtree mitchmindtree merged commit 1e9da8c into PistonDevelopers:master May 25, 2019
mitchmindtree added a commit to mitchmindtree/conrod that referenced this pull request May 25, 2019
Includes the following changes:

- A fix for list cropping in the piston backend. PistonDevelopers#1274
- Update winit and vulkano dependencies in the vulkano backend. PistonDevelopers#1278
- Improvements and fixes to vulkano backend glyph cache. PistonDevelopers#1270.
@tdaffin tdaffin deleted the fix_crop branch May 25, 2019 17:26
@tdaffin
Copy link
Contributor Author

tdaffin commented May 25, 2019

Thanks. I've just been testing the new version and all seems well...

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.

Piston window doesn't work with DropDownList
3 participants