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

Cleanup main tick #5701

Merged
merged 7 commits into from
Sep 21, 2022
Merged

Cleanup main tick #5701

merged 7 commits into from
Sep 21, 2022

Conversation

netpro2k
Copy link
Contributor

@netpro2k netpro2k commented Sep 8, 2022

This PR attempts to consolidate everything that happens in a frame into 1 spot so its easy to see the order things are executing in. It also cleans up several pending TODOs around it, namely unifying the various timers we were using and getting rid of some more globals being stored on a-scene. This doesn't yet attempt to handle the TODOs in bit-camera-sytem around turning off scene.autoUpdate (#5323). That feels like it warrants its own PR just because "matrix update bugs" are sneaky.

This is probably easiest to review commit-by-commit since I tried to break them down into logical chunks and also the intermediate commits had NOTES that might be useful to read.

Copy link
Contributor

@johnshaughnessy johnshaughnessy left a comment

Choose a reason for hiding this comment

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

lgtm

const aframeSystems = sceneEl.systems;
const hubsSystems = aframeSystems["hubs-systems"];

// TODO does anything actually ever pause the scene?
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this check existed before, and has just been moved to here, but it seems like it would be a bug for anything to be able to pause the scene. At least -- I'm not aware of anything that would be doing that on purpose.

Along similar lines, perhaps we shouldn't start the mainTickuntil DOMContentDidLoad so we can remove the check here.

@netpro2k netpro2k merged commit bab932e into master Sep 21, 2022
@netpro2k netpro2k deleted the cleanup-main-tick branch September 21, 2022 22:22
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