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

Map loading speedups #2701

Merged
merged 6 commits into from Dec 19, 2019
Merged

Conversation

Phlosioneer
Copy link
Contributor

While trying to tackle #920, I had issues loading my test files. So I found these performance improvements in the CSV decoder and the first render of the map.

I wasn't able to make any more progress in 920, so this PR is just the speedups I found on the way.

On a map of 30,000 x 30,000 or more with one layer, tiled would take
minutes (during which it was unresponsive). ~90% of that time was calling
destructors for the 900,000 QStrings allocated by `QString::split`.
Previously, loading a large map initialized the zoom to arbitrarily small
scales. During my testing, a 3000x3000 map would load in at 0.06% zoom.
Rendering all those tiles made the first draw and any user actions,
including zooming back in, very very slow.

This commit caps the number of tiles on screen to about 4,000 for the first
draw. The user is still free to zoom out, but the app will start out much
more responsive.
By manually iterating, we can avoid allocating a QVector with width*height
entries.

This speeds up loading times by 50% for 3,000 x 3,000 CSV inputs.
@Phlosioneer
Copy link
Contributor Author

Ah, subscript on QStringRef was added Qt 5.7, but we need to support Qt 5.6? I'll switch to using QStringRef::at instead.

QStringRef's [] operator isn't supported on Qt 5.6
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

That's a great improvement! No idea why I was still using the plain split function in this code... since I changed many other places to avoid turning the QStringRef into a QString. It's nice that you could even get rid of the trimming by manually parsing the thing.

However, please separate the change to the initial map size into another pull request. I'm not sure about that change since usually even 200x200 maps (so 40,000 tiles) should render at decent speeds. Limiting it to just 4000 visible tiles seems rather severe. I'd in any case want to consider it separately from speeding up the CSV parsing.

src/libtiled/mapreader.cpp Outdated Show resolved Hide resolved
src/libtiled/mapreader.cpp Outdated Show resolved Hide resolved
@Phlosioneer
Copy link
Contributor Author

Removed the map size code.

@bjorn bjorn merged commit 5278c83 into mapeditor:master Dec 19, 2019
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

2 participants