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

Refactor Application.cpp #939

Merged
merged 9 commits into from
Feb 24, 2025
Merged

Conversation

HifiExperiments
Copy link
Member

@HifiExperiments HifiExperiments commented Apr 16, 2024

Closes #931

(apologies to whoever reviews this)

Application.cpp has gotten totally out of control over the years (in many cases because of my dumb decisions). This is an attempt to corral it. I've organized the code as best I could into sections - Assets, Camera, Entities, Events, Graphics, Plugins, Setup, and UI - and divided the implementations into separate, smaller files. I completely rebuilt the includes from scratch so we only have what's necessary. I broke some of the classes into their own files. Overall, I changed very little functionally. I removed a few unused variables and functions but nearly everything is directly copy-pasted.

Testing-wise, things should just work as before. In particular, anything related to startup, login UI, and plugins.

Funding

This project is funded through NGI0 Entrust, a fund established by NLnet with financial support from the European Commission's Next Generation Internet program. Learn more at the NLnet project page.

NLnet foundation logo
NGI Zero Logo

@HifiExperiments HifiExperiments added needs CR This pull request needs to be code reviewed needs QA This pull request needs to be tested NLnet labels Apr 16, 2024

bool _profilingInitialized { false };
Copy link
Member Author

Choose a reason for hiding this comment

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

oh yeah @daleglass it didn't look like this was ever set to true, so I deleted it + the one place it was used, is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, that looks like something I screwed up.

Memory is a bit rusty right now about why I added that, but I take it PROFILE_RANGE_IF_LONGER and similar are not safe to call before some sort of initialization is done, so a _profilingInitialized=true; is supposed to go somewhere in the startup code after some profiling system init code is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

it seemed to be working ok for me without it, maybe if it's a problem during testing I can add it back?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it's simple enough that it'd be easy. I did quite a lot of rearranging of that code, and right now can't recall if maybe I just fixed this in some other way in the end.

@HifiExperiments
Copy link
Member Author

hm

/__w/overte/overte/interface/src/scripting/PerformanceScriptingInterface.cpp:14:10: fatal error: QtQML: No such file or directory
   14 | #include <QtQML>
      |          ^~~~~~~

I'll look into that...

Copy link
Member

@ksuprynowicz ksuprynowicz left a comment

Choose a reason for hiding this comment

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

This is really awesome :)
Application.cpp needed refactoring badly and this will make future work a lot easier.
I didn't find any problems, but I have few questions and I found what I think is earlier thread safety issue in the code.

@ksuprynowicz
Copy link
Member

I'm also having trouble building it on Linux using Overte-builder:
image

@ksuprynowicz
Copy link
Member

I tested it on Windows, and it asks to log in every time. A different than usual login screen is used for this:
overte-snap-by--on-2024-06-15_15-10-47
Kraftwerk world becomes entirely white after skybox loads:

overte-snap-by--on-2024-06-15_15-02-21
No audio devices are detected:
obraz

@ksuprynowicz
Copy link
Member

It also seems that login screen that it uses in VR is intended for desktop?

@ksuprynowicz
Copy link
Member

The good thing about that different login screen though is that it has option to continue anonymously. Ours is badly missing this.

@HifiExperiments
Copy link
Member Author

I believe I've fixed the audio device issue. for the login screen, it turns out I had accidentally deleted _disableLoginScreen which was always set to true. so whatever that fullscreen login page was for, it appears to have been disabled intentionally at some point. I've re-disabled it to just avoid messing with stuff, but maybe we should look into re-enabling it properly at some point. I'm not sure if this also fixed the VR login screen, if that's still a problem could you send me a screenshot?

I wasn't able to reproduce the issue in kraftwerk; I wonder if the download got corrupted or something? does it still happen if you clear your caches, including ktx cache?

@JulianGro
Copy link
Member

I gave this a test on Linux and so far, everything works fine. I tried clearing shader cache and ktx cache as well. In both scenarios, Kraftwerk renders properly.

@ksuprynowicz
Copy link
Member

I tested it again and now everything works well :)

@ksuprynowicz
Copy link
Member

Would someone like to test in VR on Windows? I don't have a VR setup at the moment. If all works well we could merge it.

@HifiExperiments
Copy link
Member Author

there are some conflicts now, I’ll probably wait until after the next release to rebase and resolve them just to avoid causing any problems

@Armored-Dragon
Copy link
Member

Would someone like to test in VR on Windows? I don't have a VR setup at the moment. If all works well we could merge it.

I did some brief testing in VR and everything seems to work as expected.

there are some conflicts now, I’ll probably wait until after the next release to rebase and resolve them just to avoid causing any problems

I will also take a look at this again after the rebase!

@HifiExperiments
Copy link
Member Author

I've rebased this and it's all ready to be reviewed + tested again!

@HifiExperiments
Copy link
Member Author

I've rebased this again and it's still ready for final testing + review!

@ksuprynowicz
Copy link
Member

I just tested it on Linux and on every start no microphone is selected. Default microphone needs to be selected every start. Output seems to be selected properly.

@ksuprynowicz
Copy link
Member

It seems that microphone issue happens only on Linux. It happens on every start and no input is selected:
image

@ksuprynowicz
Copy link
Member

It looks like the microphone issue doesn't happen in Desktop mode on Windows, but does happen in VR mode on Windows, exactly like in Desktop mode on Linux.

@ksuprynowicz
Copy link
Member

Teleport script in the tower near Overte_hub spawn point is broken, it fails with message:

[01/05 19:47:37] [CRITICAL] [overte.scriptengine] [about:Entities 1] Function call failed: "failed on line 26 column 8 with message: "Uncaught ReferenceError: Window is not defined" backtrace: ReferenceError: Window is not defined
[01/05 19:47:37] [CRITICAL] [overte.scriptengine]     at Object.enterEntity (https://aleziakurdis.github.io/inertia/teleporters/teleporter.js:26:9)
[01/05 19:47:37] [CRITICAL] [overte.scriptengine] [about:Entities 1] JS function call failed:

@ksuprynowicz
Copy link
Member

After some more testing input selection issue seems to happen sometimes in Desktop mode on Windows too.

@HifiExperiments
Copy link
Member Author

darn! thank you for testing, I'll look into those

@HifiExperiments
Copy link
Member Author

there’s definitely some weirdness around audio devices on startup. the previous fix (to the much earlier issue of the audio devices not showing up) was to reorder some initialization: 2345bcc#diff-379416a9df19174e2bf0d9d644da8269931fbe2f7f0516719eedb75f8d022b34R810

so…perhaps there’s still something wrong about the order of that versus the loading from settings

@HifiExperiments
Copy link
Member Author

HifiExperiments commented Feb 22, 2025

thanks again to @ksuprynowicz, the audio issue appears to be fixed!

I haven't looked at the teleport issue yet, if that's still an issue I will take a look this weekend, but otherwise I think this is ready for more testing actually I just gave it a try and it seems to work?

@ksuprynowicz
Copy link
Member

We did a lot of testing on the newest version of this PR, both in desktop and VR. All seems to work good :)

@ksuprynowicz ksuprynowicz added QA approved This pull request has been successfully tested and removed needs QA This pull request needs to be tested labels Feb 24, 2025
@HifiExperiments
Copy link
Member Author

amazing! this is ready to merge then from my side. 🎉

Copy link
Member

@ksuprynowicz ksuprynowicz left a comment

Choose a reason for hiding this comment

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

Everything looks good :)

@ksuprynowicz ksuprynowicz added CR approved This pull request has been successfully code reviewed and removed needs CR This pull request needs to be code reviewed labels Feb 24, 2025
@ksuprynowicz ksuprynowicz merged commit fa13c06 into overte-org:master Feb 24, 2025
6 checks passed
@HifiExperiments HifiExperiments deleted the application branch February 24, 2025 20:12
@HifiExperiments
Copy link
Member Author

HUGE thank you @ksuprynowicz for all your help with this!

@ksuprynowicz
Copy link
Member

@HifiExperiments Thank you for the refactor - it was a massive undertaking and was really needed with how big Application.cpp got XD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR approved This pull request has been successfully code reviewed NLnet QA approved This pull request has been successfully tested
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Maintenence: Refactor Application.cpp
5 participants