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 0.14.0-rc.2 #537

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

ChristopherBiscardi
Copy link

@ChristopherBiscardi ChristopherBiscardi commented Jun 7, 2024

I started updating to the 0.14 rc release. Each commit has a link to a PR that enacted the change.

  • 12997: rename multi-threaded feature to multi_threaded
  • 12827 RenderAssets<Image> is now RenderAssets<GpuImage>
  • 12732 FloatOrd is now in bevy_math
  • 12889 Gpu Frustum Culling removed the dynamic_offset of Transparent2d
  • 12453 Render Phases are now binned and sorted.
  • 9202 SubApp access has changed from Result to Option
  • 11698 GpuImage now uses UVec2s
  • 12791 the primitive_topology field on GpuMesh was removed.
  • 12216 introduced an argument &mut MeshVertexBufferLayouts to get_mesh_vertex_buffer_layout, which bevy_ecs_tilemap calls in RenderChunk2d::prepare
  • 12163 bevy_color was created and Color handling has changed. Specifically Color::as_linear_rgba_f32 has been removed and we have to go through LinearRgb using .linear().
  • 12582 divided VisibleEntities into separate lists. Specify the entity type we want
  • 9202 changed world access to functions. relevent line
    • This also surfaced 12655 which removed Into<AssetId<T>> for Handle<T>. using a reference or .id() is the solution here.
  • 12551 Removed World::cell()... but we don't need it or the replacement anyway.
  • 12582 check_visibility must be implemented for the "renderable" tilemap entities
  • 13289 introduced matrix naming changes for view.view_proj

Current Status

Code compiles. Examples error out with bevyengine/bevy#13728

old history

Once the fix in the above issue is added (the zstd + bevy_pbr features to the dev-dependencies bevy), the examples run but don't render

screenshot-2024-06-07-at-01 28 08@2x

Basic example is now rendering

screenshot-2024-06-07-at-11 27 38@2x

* [12997](bevyengine/bevy#12997): rename `multi-threaded` to `multi_threaded`
[12889](bevyengine/bevy#12889) Gpu Frustum Culling removed the dynamic_offset of Transparent2d and it became `extra_index` with the special value `PhaseItemExtraIndex::NONE`, which indicates the `None` that was here previously
[11698](bevyengine/bevy#11698) changed `GpuImage::size` to `UVec2`.

Right above this, `Extent3d` does the same thing, so I'm taking a small leap and assuming can `as`.
[12791](bevyengine/bevy#12791) the `primitive_topology` field on `GpuMesh` was removed in favor of `key_bits` which can be constructed using `BaseMeshPipeline::from_primitive_topology`
[12216](bevyengine/bevy#12216) introduced an argument `&mut MeshVertexBufferLayouts` to `get_mesh_vertex_buffer_layout`, which bevy_ecs_tilemap calls in `RenderChunk2d::prepare`
[12163](bevyengine/bevy#12163) bevy_color was created and Color handling has changed. Specifically Color::as_linear_rgba_f32 has been removed.

LinearRgba is now its own type that can be accessed via [`linear()`](https://docs.rs/bevy/0.14.0-rc.2/bevy/color/enum.Color.html#method.linear) and then converted.
[12582](bevyengine/bevy#12582) divided `VisibleEntities` into separate lists. So now we have to specify which kind of entity we want. I think we want the Mesh here, and I think we can get rid of the `.index` calls on Entity since Entity [already compares bits](https://docs.rs/bevy_ecs/0.14.0-rc.2/src/bevy_ecs/entity/mod.rs.html#173) for optimized codegen purposes. Waiting to do that until the other changes are in though so as to not change functionality until post-upgrade.
- [9202](bevyengine/bevy#9202) changed world access to functions. [relevent line](https://github.com/bevyengine/bevy/pull/9202/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97fR640)
- This also surfaced [12655](bevyengine/bevy#12655) which removed `Into<AssetId<T>>` for `Handle<T>`. using a reference or .id() is the solution here.
In [12551](bevyengine/bevy#12551) `WorldCell` was removed.

...but it turns out we don't need it or its replacement anyway.
@@ -1,7 +1,7 @@
[package]
name = "bevy_ecs_tilemap"
description = "A tilemap rendering plugin for bevy which is more ECS friendly by having an entity per tile."
version = "0.12.0"
Copy link
Author

Choose a reason for hiding this comment

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

bevy_ecs_tilemap 13 never released to crates.io, so I moved this to 14 because I think versions track bevy versions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the pattern in recent releases is a coincidence.

Copy link
Author

Choose a reason for hiding this comment

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

I can change the version to anything relevant. It seems like the Bevy project is hoping people will release rc candidates, although I don't know if anyone with publish access has the time for that at the moment for this crate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think StarArawn generally handles release prep himself.

@ChristopherBiscardi ChristopherBiscardi marked this pull request as ready for review June 7, 2024 08:37
@ChristopherBiscardi ChristopherBiscardi changed the title [draft] Update to 0.14.0-rc.2 Update to 0.14.0-rc.2 Jun 7, 2024
@rparrett
Copy link
Collaborator

rparrett commented Jun 7, 2024

I think the current migration for:

12582 divided VisibleEntities into separate lists. Specify the entity type we want

Is part of the problem at the moment. I suspect we need to add our own visibility type and check system: check_visibility::<WithTilemap> or whatever.

But there may be something else going on too.

As a result of [12582](bevyengine/bevy#12582) `check_visibility` must be implemented for the "renderable" tilemap entities. Doing this is trivial by taking advantage of the
existing `check_visibility` type arguments, which accept a [`QF: QueryFilter + 'static`](https://docs.rs/bevy/0.14.0-rc.2/bevy/render/view/fn.check_visibility.html).

The same `QueryFilter`` is used when checking `VisibleEntities`. I've chosen `With<TilemapRenderSettings` because presumably if the entity doesn't have a `TilemapRenderSettings` then it will not be rendering, but this could be as sophisticated or simple as we want.

For example `WithLight` is currently implemented as

```rust
pub type WithLight = Or<(With<PointLight>, With<SpotLight>, With<DirectionalLight>)>;
```
[13289](bevyengine/bevy#13489) introduced matrix naming changes, including `view_proj` which becomes `clip_from_world`
@ChristopherBiscardi
Copy link
Author

@rparrett yeah, it was the check_visibility implementation that needed to be added. I checked a number of examples (but not all yet) and so far every one I've checked has rendered.

basic

screenshot-2024-06-07-at-11 27 38@2x

3d_iso

screenshot-2024-06-07-at-11 30 10@2x

@ChristopherBiscardi
Copy link
Author

I've updated color usage in examples as minimally as possible to make the tests and examples run, but I'd like to come back after this merges to update all of the example's colors to the new palettes and make some of the contrast more readable in a future PR if that's ok.

@rparrett
Copy link
Collaborator

rparrett commented Jun 7, 2024

I'll run through this checklist tonight:

  • Check all examples on native (macos over here) (cargo-examples is nice)
  • Check all examples on native (with features=atlas)
  • Spot check basic, custom_shader, tiled, ldtk with --target=wasm32-unknown-unknown --features=atlas
  • Spot check basic, custom_shader, tiled, ldtk with --target=wasm32-unknown-unknown --features=bevy/webgpu

edit: Looking good

@rparrett
Copy link
Collaborator

rparrett commented Jun 7, 2024

I'd like to come back after this merges to update all of the example's colors to the new palettes and make some of the contrast more readable in a future PR if that's ok.

That would be nice. They have been in a particularly bad state since Bevy changed the default clear color.

@ChristopherBiscardi
Copy link
Author

ChristopherBiscardi commented Jun 7, 2024

for windows, since you're doing mac later:

  • Check all examples on native (macos over here) (cargo-examples is nice)
  • Check all examples on native (with features=atlas)
    • crashes on texture_container example, which seems to be the expected behavior as this combination is not supported
  • Spot check basic, custom_shader, tiled, ldtk with --target=wasm32-unknown-unknown --features=atlas

basic example has an issue in bevy_core_pipeline

error[E0432]: unresolved import `crate::core_3d::DEPTH_TEXTURE_SAMPLING_SUPPORTED`
  --> C:\Users\chris\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_core_pipeline-0.14.0-rc.2\src\dof\mod.rs:59:19
   |
59 |         Camera3d, DEPTH_TEXTURE_SAMPLING_SUPPORTED,
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `DEPTH_TEXTURE_SAMPLING_SUPPORTED` in `core_3d`
   |
note: found an item that was configured out
  --> C:\Users\chris\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_core_pipeline-0.14.0-rc.2\src\core_3d\mod.rs:53:11
   |
53 | pub const DEPTH_TEXTURE_SAMPLING_SUPPORTED: bool = false;
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: found an item that was configured out
  --> C:\Users\chris\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_core_pipeline-0.14.0-rc.2\src\core_3d\mod.rs:63:11
   |
63 | pub const DEPTH_TEXTURE_SAMPLING_SUPPORTED: bool = true;
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  • Spot check basic, custom_shader, tiled, ldtk with --target=wasm32-unknown-unknown --features=bevy/webgpu

all webgpu examples build. It doesn't look like we have a built-in setup to run them with cargo run so I did not run them.


also cargo-examples is amazing. very useful, thanks for the recommendation.

@ChristopherBiscardi
Copy link
Author

wasm builds also require bevy/webgl2 because the const is also gated by bevy_core_pipeline/webgl. This was added in 13418

〉cargo build --example custom_shader --target wasm32-unknown-unknown --features=atlas --features=bevy/webgl2

@rparrett
Copy link
Collaborator

rparrett commented Jun 7, 2024

Ah, that's why I suggested adding back webgl2 to dev-dependencies above.

@ChristopherBiscardi
Copy link
Author

oh you did? I must have missed that (and can't find it either)

Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@@ -1,7 +1,7 @@
[package]
name = "bevy_ecs_tilemap"
description = "A tilemap rendering plugin for bevy which is more ECS friendly by having an entity per tile."
version = "0.12.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the pattern in recent releases is a coincidence.

@rparrett
Copy link
Collaborator

rparrett commented Jun 7, 2024

Oops, I had left my review pending.

ChristopherBiscardi and others added 2 commits June 7, 2024 13:15
Co-authored-by: Rob Parrett <robparrett@gmail.com>
Co-authored-by: Rob Parrett <robparrett@gmail.com>
@ChristopherBiscardi
Copy link
Author

confirmed that fixed the build on my end after committing the review changes

@ChristopherBiscardi
Copy link
Author

alright, I finished up the last of the clippy fixes too so this should be ready for you when you get to it later. Unless there's an issue I don't think there's anything left to do in this PR.

Cargo.toml Show resolved Hide resolved
Co-authored-by: Rob Parrett <robparrett@gmail.com>
Copy link
Collaborator

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

Looks good to me! Awesome work.

@rparrett
Copy link
Collaborator

Opened ChristopherBiscardi#1 for a minor touchup removing the lint-allow from the ldtk/tiled examples.

@ChristopherBiscardi
Copy link
Author

merged it in. Committed the removal of the second allow lint as well.

@ChristopherBiscardi
Copy link
Author

ChristopherBiscardi commented Jun 13, 2024

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