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

Bevy v0.15.0 #87

Merged
merged 28 commits into from
Feb 3, 2025
Merged

Bevy v0.15.0 #87

merged 28 commits into from
Feb 3, 2025

Conversation

elaye
Copy link
Contributor

@elaye elaye commented Dec 11, 2024

#86

Copy link
Member

@simbleau simbleau left a comment

Choose a reason for hiding this comment

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

This PR looks great!

A few questions, I'm still trying to answer:

  • Instead of <MyHandleComponent>.0 everywhere, I think it would be nicer to use .id(). This requires MyComponentHandle to derive Deref and DerefMut
  • Is there yet a convention on what to name these newtypes for Handles? I've seen several cases where Bevy renamed them to have "2d" at the end, e.g. MeshMaterial2d

src/render/systems.rs Outdated Show resolved Hide resolved
@simbleau
Copy link
Member

simbleau commented Dec 11, 2024

Another note, which is somewhat unrelated, is that certain logic should be able to be eliminated or optimized with new features in 0.14/0.15.

Specifically, the spawn_playheads function exists to attach a playhead to a newly added Lottie. This is a good job for required components (or observers). Ideally I really want to split Svg and Lottie into their own types, rather than both being used by VelloAsset, but that feels like a different PR.

@simbleau simbleau marked this pull request as draft December 11, 2024 15:22
@simbleau
Copy link
Member

AH, one more catch: The VelloAssetBundle, VelloTextBundle and VelloSceneBundle now no longer require global transform, view visibility, or inherited visibility. Those are required components brought in by Transform and Visibility now.

@simbleau simbleau mentioned this pull request Dec 11, 2024
@elaye
Copy link
Contributor Author

elaye commented Dec 11, 2024

Regarding the Handle names, I don't think there is a convention yet. If we look at an example of the migration from bevy, they changed struct Mesh2dHandle(pub Handle<Mesh>); to struct Mesh2d(pub Handle<Mesh>); but it's hard to find an equivalence with VelloAsset and its handle. We could use VelloAsset for the handle name but we would have to come up with another name for the wrapped struct, that's why I used VelloAssetHandle as it seemed simpler.

As for the migration to bevy 0.15 features, you're obviously right! However I didn't want to make the PR too big. I figured a first PR could make it work with 0.15 and a second PR could make it more idiomatic :)

@simbleau
Copy link
Member

simbleau commented Dec 11, 2024

Btw I got some errors that the vello_svg::vello::Scene and vello::Scene versions are mismatched. Not sure if you see those too. Can't tell because our Cargo.lock isn't staged.

@DJMcNab or @xStrom - Was it determined that all linebender projects will stage the Cargo.lock?

@elaye
Copy link
Contributor Author

elaye commented Dec 11, 2024

Yes I have the same errors when I run cargo check --all. It seems that vello::Scene and vello_svg::vello::Scene come from two different versions of vello, probably because I used unreleased versions, making things inconsistent.

@RobertBrewitz
Copy link
Contributor

RobertBrewitz commented Dec 12, 2024

I thought I'd chip in a little as I dabbled with this during 0.15 release candidate process, and I remember I was stuck for a few hours on why extracted entities kept piling up duplicates in the render world; As I did not know about this new retained render world feature in 0.15.

Unless 1) some sort of resource structure is implemented to keep app world entities and render world entities in sync or 2) spawn extracted entities with the TemporaryRenderEntity component so they despawn after every frame, they'll keep being spawned every frame and never be removed from the render world.

Edit: Deriving ExtractComponent onto VelloScene and VelloTextSection and using the ExtractComponentPlugin might be enough to make it work with minimal effort.

Hope it helps!

@DJMcNab
Copy link
Member

DJMcNab commented Dec 12, 2024

linebender/rfcs#5 led to the agreement that we would commit Cargo.lock.

@msvbg
Copy link
Contributor

msvbg commented Jan 3, 2025

Hey, what’s the state of this PR? Is anyone pushing it forward?

@simbleau
Copy link
Member

simbleau commented Jan 5, 2025

Hey, what’s the state of this PR? Is anyone pushing it forward?

Yes, there is a longer-than-expected delay in vello 0.4.0 coming out which is holding this PR up.

Bevy 0.15 depends on wgpu 23, and the last version of vello (0.3) uses a prior version of wgpu.

@RobertBrewitz
Copy link
Contributor

I've been playing around a bit more with a fork and at the time of writing, we have to update the specialize function to get transparent base_color working; Relates to linebender/vello#549 which is not in the 0.4.0 milestone any more for some reason.

fn specialize(
        descriptor: &mut RenderPipelineDescriptor,
        _layout: &MeshVertexBufferLayoutRef,
        _key: Material2dKey<Self>,
    ) -> Result<(), SpecializedMeshPipelineError> {
        if let Some(target) = descriptor.fragment.as_mut() {
            let mut_targets = &mut target.targets;
            if let Some(Some(target)) = mut_targets.get_mut(0) {
                target.blend = Some(vello::wgpu::BlendState::ALPHA_BLENDING);
            }
        }

        let formats = vec![
            // Position
            VertexFormat::Float32x3,
            VertexFormat::Float32x2,
        ];

        let vertex_layout =
            VertexBufferLayout::from_vertex_formats(VertexStepMode::Vertex, formats);

        descriptor.vertex.buffers = vec![vertex_layout];

        Ok(())
    }

@simbleau
Copy link
Member

Velato is very close, which will unblock this.
linebender/velato#49

@simbleau
Copy link
Member

simbleau commented Jan 28, 2025

I've been playing around a bit more with a fork and at the time of writing, we have to update the specialize function to get transparent base_color working; Relates to linebender/vello#549 which is not in the 0.4.0 milestone any more for some reason.

fn specialize(
        descriptor: &mut RenderPipelineDescriptor,
        _layout: &MeshVertexBufferLayoutRef,
        _key: Material2dKey<Self>,
    ) -> Result<(), SpecializedMeshPipelineError> {
        if let Some(target) = descriptor.fragment.as_mut() {
            let mut_targets = &mut target.targets;
            if let Some(Some(target)) = mut_targets.get_mut(0) {
                target.blend = Some(vello::wgpu::BlendState::ALPHA_BLENDING);
            }
        }

        let formats = vec![
            // Position
            VertexFormat::Float32x3,
            VertexFormat::Float32x2,
        ];

        let vertex_layout =
            VertexBufferLayout::from_vertex_formats(VertexStepMode::Vertex, formats);

        descriptor.vertex.buffers = vec![vertex_layout];

        Ok(())
    }

Noted! Looking into it.

@simbleau
Copy link
Member

There appears to be a regression on this branch.

The images are not getting cleared every frame, see cargo run -p lottie

image

@simbleau
Copy link
Member

After some digging Im still not sure why the frame buffer doesnt clear

@RobertBrewitz
Copy link
Contributor

RobertBrewitz commented Jan 29, 2025

After some digging Im still not sure why the frame buffer doesnt clear

Is it that you are not adding TemporaryRenderEntity to the extracted entities in the extract systems?

Edit: this would be the quick fix I suppose

@simbleau
Copy link
Member

After some digging Im still not sure why the frame buffer doesnt clear

Is it that you are not adding TemporaryRenderEntity to the extracted entities in the extract systems?

Edit: this would be the quick fix I suppose

Ah, beautiful!
image

Infact this was the issue.
https://bevyengine.org/learn/migration-guides/0-14-to-0-15/#spawning-entities-in-the-render-world

@simbleau simbleau marked this pull request as ready for review February 2, 2025 19:03
Copy link
Member

@simbleau simbleau left a comment

Choose a reason for hiding this comment

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

This looks ready to me (there seems to be a new CI issue though)

@simbleau simbleau added this pull request to the merge queue Feb 3, 2025
Merged via the queue into linebender:main with commit ae710fe Feb 3, 2025
3 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Feb 6, 2025
Builds upon #87. Merge that first.

Closes #89.

---------

Co-authored-by: Elie Génard <[email protected]>
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.

6 participants