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

Put move_skybox into CoreStage::PostUpdate #9

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

TGRCdev
Copy link
Contributor

@TGRCdev TGRCdev commented Oct 24, 2021

I first used this plugin in combination with the bevy_flycam plugin, but when i wrote a simple test program, occasionally the skybox would rapidly jitter whenever the camera moved.

skybox.mp4

I fixed this by creating a stage label SkyboxStage, which is a single system stage that runs after CoreStage::Update and only contains the move_skybox system. (This PR now puts move_skybox into CoreStage::PostUpdate instead of its own stage.) My rationale is that any method that moves the camera will most likely be within the Update stage, and this change will improve bevy_skybox as a drop-in plugin without any camera system needing to be aware of its ordering.

I initially tried to fix this by going into bevy_flycam and adding .before("skybox") to the systems that moved the camera, but this output the error "system wants to run before unknown label 'skybox'" and did not fix the issue. Thus, I believe that this is the correct fix to this problem.

@alice-i-cecile
Copy link

Moving this plugins' systems to CoreStage::PostUpdate will achieve a similar effect of "correct behavior by default" and be less intrusive.

More generally, this plugin should be exporting a nice typed system label or two to allow users to control within stage ordering.

Thanks for bringing this up, this is a great simple and concrete example of the problems that bevyengine/rfcs#33 tries to address.

@TGRCdev
Copy link
Contributor Author

TGRCdev commented Oct 24, 2021

My bad! I assumed that CoreStage::PostUpdate would execute after the frame rendered, but this assumption was wrong. Changed and squashed.

@TGRCdev
Copy link
Contributor Author

TGRCdev commented Oct 24, 2021

Also added two typed system labels, SkyboxInitSystemsLabel for the startup systems and SkyboxSystemLabel for move_skybox

This change should be made in a separate PR

@TGRCdev TGRCdev changed the title Put move_skybox into its own stage after CoreStage::Update Put move_skybox into CoreStage::PostUpdate Oct 24, 2021
@jomala jomala merged commit 5eb7eea into jomala:master Oct 25, 2021
@jomala
Copy link
Owner

jomala commented Oct 25, 2021

Thanks for the contribution, @TGRCdev !

@TGRCdev TGRCdev deleted the ordering-fix branch October 25, 2021 15:33
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.

3 participants