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

Further optimization of OBJ loading #3655

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

10110111
Copy link
Contributor

@10110111 10110111 commented Mar 4, 2024

Description

It appears that QString::toInt() has become much slower in Qt 6.4.1 compared to Qt 5.15.4. For this reason I switched to std::from_chars for ints. Some more improvement comes from doing this for floats. Additionally, I removed the use of QString for parsing, so that it all now works with usual char units, which is more friendly to std::from_chars calls.

The results are as follows in the test where I simply call StelOBJ::load() from main to avoid any variability due to multithreading:

  • Qt 6.4 : 16.5 s -> 3.4 s
  • Qt 5.15: 4.9 s -> 3.9 s

In the real-life scenario of loading in background during rendering the final times of loading are about 12 s (both in Qt5 and Qt6) down from about 30 s in Qt6 and 16 s in Qt5.

The only problem is that our macOS CI seems to use a relatively old compiler whose C++ standard library doesn't implement std::from_chars for floating-point types, and this breaks the build. One fix is to switch to a newer compiler if possible, and another is to employ fast_float library when the C++ standard library doesn't provide these functions.

Copy link

github-actions bot commented Mar 4, 2024

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files.

@10110111 10110111 force-pushed the obj-loading-optimization branch from ec0abad to 8c31671 Compare March 4, 2024 18:03
Copy link
Member

@gzotti gzotti left a comment

Choose a reason for hiding this comment

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

Yess!! 42s->33s is a further great improvement, thank you. I tested with textured, untextured and a mixed (partly textured, partly simple material) scene. Now, @alex-w , is there a better compiler for the CI for Mac?

Copy link
Member

@alex-w alex-w left a comment

Choose a reason for hiding this comment

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

We can switch to macOS 13 (clang 15) for CI, but I see strange troubles for unit tests for these instances…

P.S. Or we can switch to experimental images with macOS 14.

@alex-w alex-w added this to the 24.1 milestone Mar 5, 2024
@gzotti
Copy link
Member

gzotti commented Mar 5, 2024

Would that influence required minVersion of macOS? If not, please go for it!
Some gcc versions emit numerous warnings. We should try to fix those soon to avoid future problems. E.g., IIRC, some type casting orgies were still not silent in the libtess. Maybe some test are also affected by compiler versions?

@alex-w
Copy link
Member

alex-w commented Mar 5, 2024

Would that influence required minVersion of macOS?

It's depended on version of Qt and target platform. Currently, we use macOS 11.0+ for Qt6-based packages, but I've built these packages in macOS 14.

@10110111 10110111 merged commit 7362d36 into Stellarium:master Mar 5, 2024
11 of 12 checks passed
@10110111 10110111 deleted the obj-loading-optimization branch March 6, 2024 10:56
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Mar 10, 2024
Copy link

Hello @10110111!

Please check the fresh version (development snapshot) of Stellarium:
https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Mar 26, 2024
Copy link

Hello @10110111!

Please check the latest stable version of Stellarium:
https://github.com/Stellarium/stellarium/releases/latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants