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

Update to Bevy 0.10 #390

Merged
merged 15 commits into from
Mar 17, 2023
Merged

Update to Bevy 0.10 #390

merged 15 commits into from
Mar 17, 2023

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Feb 12, 2023

Hello,
this updates the code base to bevy version 0.10 and should be ready to merge.

@rparrett
Copy link
Collaborator

rparrett commented Feb 16, 2023

Oops, I just did this migration too.

I have a couple notes from my efforts (was about half done migrating the examples when I thought to check here) that look like they could be useful in this PR:

  1. There are a couple spots in the examples where
    .add_startup_system_to_stage(StartupStage::PostStartup, whatever)
    is being abused to get a commands flush between startup systems that can now be replaced with stuff like
    .add_startup_systems((a, apply_system_buffers, b).chain())
  2. hex_neighbors and mouse_to_tile should use the new viewport_to_world_2d function.

@rparrett
Copy link
Collaborator

  1. Bevy code post-[Merged by Bors] - Flatten render commands bevyengine/bevy#6885 seems to prefer e.g.
    -        image_bind_groups: Res<'w, ImageBindGroups>,
    +        image_bind_groups: SystemParamItem<'w, '_, Self::Param>,
    Although I couldn't explain why (except that reusing Self::Param seems tidy.)

@geieredgar
Copy link
Contributor Author

I have updated the code to bevy commit bevyengine/bevy@64b0f19 and incorporated all suggested changes.

@geieredgar geieredgar changed the title Update to latest bevy version Update to latest bevy version (0.10) Mar 6, 2023
@geieredgar geieredgar mentioned this pull request Mar 6, 2023
3 tasks
@geieredgar geieredgar changed the title Update to latest bevy version (0.10) Update to latest bevy 0.10 Mar 6, 2023
@geieredgar geieredgar changed the title Update to latest bevy 0.10 Update to bevy 0.10 Mar 6, 2023
@geieredgar geieredgar changed the title Update to bevy 0.10 Update to Bevy 0.10 Mar 6, 2023
@rparrett
Copy link
Collaborator

rparrett commented Mar 8, 2023

I opened a PR against your branch to fix a warning when using the atlas feature.

Other than that, for what it's worth, this looks good to me. Tested all the examples with/without atlas, and a few on wasm.

Copy link
Sponsor Contributor

@Trouv Trouv left a comment

Choose a reason for hiding this comment

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

I think this is pretty close - though I admit some of the deeper rendering changes are over my head

examples/hex_neighbors.rs Outdated Show resolved Hide resolved
examples/mouse_to_tile.rs Outdated Show resolved Hide resolved
src/render/mod.rs Show resolved Hide resolved
@rparrett
Copy link
Collaborator

It's a minor thing, but if we're adding another set, I personally think the behavior would be more obvious/self-documenting/robust if it's a SpawnTilemapSet.

.add_startup_systems((spawn_tilemap, apply_system_buffers).chain().in_set(SpawnTilemapSet))
.add_startup_systems((spawn_tile_labels, spawn_map_type_label).after(SpawnTilemapSet))

It seems more likely that we would add additional systems that rely on the Tilemap having been spawned than the other way around.

@geieredgar
Copy link
Contributor Author

Ok, I've replaced SpawnLabelsSet with SpawnTilemapSet and fixed the redundant .before() call I missed earlier in the hex_neighbor example.

Copy link
Sponsor Contributor

@Trouv Trouv left a comment

Choose a reason for hiding this comment

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

LGTM. I also checked all the examples w/ default features and they seemed fine. Just expressing my approval, I can't merge it

Copy link

@Sierra100010 Sierra100010 left a comment

Choose a reason for hiding this comment

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

Good

@hammackj
Copy link

Any eta on when this will get merged? Loving this map =)

@m0xsec
Copy link

m0xsec commented Mar 15, 2023

+1 on supporting this being merged when possible. thanks all!

@arbrog arbrog mentioned this pull request Mar 17, 2023
Copy link
Collaborator

@bzm3r bzm3r left a comment

Choose a reason for hiding this comment

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

Thanks everyone for your patience on this! I am going to speak to @StarArawn to give write-access to @rparrett

@bzm3r bzm3r merged commit 41628ce into StarArawn:main Mar 17, 2023
@HyperAlch
Copy link

When will update your crate to 0.10 now that this has been merged?

image

It still says 0.9

@nxsaken
Copy link

nxsaken commented Mar 21, 2023

PR #399 is about the new release. Meanwhile, you can specify the main branch in your Cargo.toml.

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.

10 participants