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 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.
@btzy btzy force-pushed the btzy:redraw-linkgraph-zoom branch from ac83367 to 3d5cbbe Jan 1, 2019
Copy link
Member

LordAro left a comment

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

btzy commented Jan 13, 2019

Maybe I'll try this next week.

@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Feb 4, 2019

Closed in favour of #7080

@LordAro LordAro closed this Feb 4, 2019
@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Feb 4, 2019

Or maybe not, needs more thought

@LordAro LordAro reopened this Feb 4, 2019
@btzy

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

nielsmh commented Feb 23, 2019

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

@nielsmh nielsmh closed this Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.