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

Fix #7004: Redraw linkgraph overlay correctly after zoom #7005

Closed
wants to merge 1 commit into from

Conversation

btzy
Copy link
Contributor

@btzy btzy commented Dec 31, 2018

Previously, when the user zooms in or out, the linkgraph overlay was recalculated before the zoom. This caused some stations and edges to be missing from the visible region of the viewport.

Thanks for this enjoyable game!

Previously, when the user zooms in or out, the linkgraph overlay was recalculated before the zoom.  This caused some stations and edges to be missing from the visible region of the viewport.
Also used LINKGRAPH_DELAY when zooming, for consistency with scrolling and resizing.
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really follow how this would work. As far as I can tell the changes here would mean the overlay would be drawn less often, rather than more often.

I wonder whether the extra parameter is the correct solution anyway. In my mind, zooming/scrolling the viewport should not need to care about whether to redraw the overlay at all. Maybe it would be acceptable to just draw it in "all" cases and sacrifice a little bit of efficiency?

@btzy
Copy link
Contributor Author

btzy commented Jan 6, 2019

Thanks for reviewing my PR.

At main_gui.cpp lines 200-202 (of master), ScrollWindowTo is called (which in turn calls RebuildViewportOverlay directly) so the cache (that keeps tract of the visible part of the cargo flow legend) is rebuilt. But this is done before actually zooming with DoZoomInOutWindow, so the cache only stores the visible objects before the zoom out.

To fix, we need to somehow call RebuildViewportOverlay after the zoom is actually done (so the rebuilding code will get to see the new zoom level).

The simplest solution (in my opinion) is to call RebuildViewportOverlay after line 202 (perhaps also using the return value of ScrollWindowTo and DoZoomInOutWindow to determine if the position and scale was actually changed).

However, in MainWindow::OnScroll and MainWindow::OnResize it seems that the cargo flow legend is purposely delayed for a short while (of LINKGRAPH_DELAY ticks) before updating, presumably because it takes a non-negligible amount of time to iterate all edges in the linkgraph for large games, which would make scrolling/resizing look choppy if called immediately when scrolling/resizing. It then seems logical to add the same delay to MainWindow::OnMouseWheel too, because we probably don't want choppy zooming as well. So to prevent choppy zooming, we have to prevent ScrollWindowTo from rebuilding the cache if it was called due to moving the mouse wheel, but still rebuild immediately if it was called due to jumping directly to a particular location (e.g. when jumping to the location of a vehicle)... and hence the boolean flag.

Maybe it would be acceptable to just draw it in "all" cases and sacrifice a little bit of efficiency?

Because the original code already had the short delay, I assumed that someone tried drawing it in all cases and realised that it was too slow.

Actually, I wonder if the current ScrollWindowTo (before my PR) should be calling RebuildViewportOverlay directly at all. I think it might be better to set an invalidation flag, then have the drawing code (perhaps UpdateViewportPosition) check that flag to determine whether it should rebuild the cache. Then we can just set the flag for both ScrollWindowTo and DoZoomInOutWindow, without worrying about duplication. Maybe to get rid of LINKGRAPH_DELAY, we can also add a bit of additional buffer area around the visible area, so that we only need to rebuild the cache when we need to draw things that are outside the additional buffer frame?

@LordAro
Copy link
Member

LordAro commented Jan 10, 2019

Thanks for the detailed explanation

Actually, I wonder if the current ScrollWindowTo (before my PR) should be calling RebuildViewportOverlay directly at all. I think it might be better to set an invalidation flag, then have the drawing code (perhaps UpdateViewportPosition) check that flag to determine whether it should rebuild the cache. Then we can just set the flag for both ScrollWindowTo and DoZoomInOutWindow, without worrying about duplication. Maybe to get rid of LINKGRAPH_DELAY, we can also add a bit of additional buffer area around the visible area, so that we only need to rebuild the cache when we need to draw things that are outside the additional buffer frame?

I'm sure you know that the simplest solutions are not always the best! This sounds good, and a cleaner way of doing things - try it and see how it turns out? If nothing else a comment can be added explaining why it's the way that it is :)

@btzy
Copy link
Contributor Author

btzy commented Jan 13, 2019

Maybe I'll try this next week.

@LordAro
Copy link
Member

LordAro commented Feb 4, 2019

Closed in favour of #7080

@LordAro LordAro closed this Feb 4, 2019
@LordAro
Copy link
Member

LordAro commented Feb 4, 2019

Or maybe not, needs more thought

@LordAro LordAro reopened this Feb 4, 2019
@btzy
Copy link
Contributor Author

btzy commented Feb 6, 2019

@LordAro This PR has the "waiting on author" tag, but I'm not in the process of writing any code for this (because imo it should be superseded by #7080). Am I expected to do anything?

@nielsmh
Copy link
Contributor

nielsmh commented Feb 23, 2019

This is not the correct solution. Either #7080 or #7265.

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.

None yet

4 participants