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

3D support through billboard rendering with instancing #46

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

Conversation

Rayoule
Copy link
Contributor

@Rayoule Rayoule commented Jun 28, 2023

Hello! 3D update!
That was quite a piece of work but here it is !

This PR adds many many things, all related to 3D compatibility:

  • render.rs and instancing.wgsl both contains all the rendering stuff. Note that I didnt use the bevy render graph api because I didn't understood how to achieve instancing with it, and an instancing example was already existing.
  • Modifications were made to the ParticleSystem, 3D can be enabled on it with the new field: render_type.
  • Also a new field of ParticleSystem: align_with_velocity enables particles to be aligned with they're current velocity.
  • New basic shapes that provide a 3D transform have been added.
  • A new example particles_3d.rs shows how this works
  • Plus some little modifications here and there

With this update comes a few problems:

  • Texture atlas is not supported when using 3d billboard rendering
  • The sorting algorythm gets buggy over time for an unknown reason. I tried a more simple sorting method (which is quite moire expensive) but with not effect, so I kept it commented and used my optimized (no data copy, calculate the square distance only once for each particle).
    It does make things work better (try disabling it), but if you pay attention, the arrow particles of the 3d example will start to get messy after a few seconds, and I have no idea why.
    If someone could check it out it could be really cool, as I am out of ideas for this.
  • The alignment in 3D is not exactly hjow you would expect it. It is slightly off for an unknown reason (see instancing.wgsl)

Well I hope you'll like it !

Raoul Desmarest and others added 30 commits March 14, 2023 13:02
Add a new drag.rs example
Update example/velocity_modifiers.rs
Add PrecalculatedParticleVariables
# Conflicts:
#	examples/basic.rs
#	src/components.rs
#	src/systems.rs
…ic struct CurvePoint.

In VelocityModifier, rename ScalarAcceleration to Scalar, and Constant to Vector.
Add VectorOverTime and replace Vector(Vec3) by Vector(VectorOverTime) in VelocityModifier.
Fix example/shape_emitter.rs lifetime so its nicer and we can see the gradient.
Add 3D and billboard material features
Copy link
Collaborator

@theanti9 theanti9 left a comment

Choose a reason for hiding this comment

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

@Rayoule Wow! Awesome work. I've taken a cursory look at this and left a few initial comments but this will take me a few days to go through fully. I appreciate the obviously massive effort here and will try to get through it soon.

src/render.rs Outdated
extracted_instance_data.instance_data.swap(i, swap_idx);
indices.swap(i, swap_idx);
}
}
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 this could be done in linear time by preallocating a vec of the right size and setting the value at the specified index.

Something like this:

let instance_data = Vec::with_capacity(indices.len());
instance_data.resize(indices.len(), ParticleBillboardInstanceData::default());
for (i in 0..indices.len()) {
    instance_data[indices[i]] = extracted_instance_data.instance_data[i];
}

And then pass instance_data to the buffer.

We are double allocating, but may still be faster. A lot less swapping and iteration.

label: Some("instance data buffer"),
contents: { bytemuck::cast_slice(extracted_instance_data.instance_data.as_slice()) },
usage: BufferUsages::VERTEX | BufferUsages::COPY_DST,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with the bevy specifics so it's very possible I'm missing something here, but with wgpu buffers, we probably don't want to recreate this buffer every frame. We should be able to store the created buffer (maybe wrapped in a Resource?), and instead update its contents every frame, which will be a good bit more efficient.

Sizing the buffer would be complicated though. Not sure how easy it'd be to plumb through but technically the max_particles of all alive 3d particle systems would be the max, and then we can just slice the buffer on the render call, with however many were actually filled that frame.

We'd still have to resize the buffer whenever that number changes though.

};

// Create the bind group for the particle system
let ps_bind_group = render_device.create_bind_group(&BindGroupDescriptor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same idea here, I think we can avoid creating these every frame.

@Rayoule
Copy link
Contributor Author

Rayoule commented Jun 29, 2023

I don't understand what is wrong with that test

@abnormalbrain
Copy link
Owner

@Rayoule rust-lang/rust#113152 Looks like cargo-udeps is broken/incompatible with the version of proc-macros2 being used by bevy right now. I'll be disabling that particular test as part of the 0.11 upgrade.

@abnormalbrain
Copy link
Owner

I've been thinking about this and wondering if we should gate the extra dependencies and extra pipeline stuff behind a cargo feature like 3d or billboards or something like that. There is a measurable performance hit to the basic example with this change, likely due to the pipeline changes, even though it's not using anything 3d, so I think it would be good to have it be optional in a way that doesn't impact users that don't need it.

@Rayoule
Copy link
Contributor Author

Rayoule commented Jul 14, 2023

I've been thinking about this and wondering if we should gate the extra dependencies and extra pipeline stuff behind a cargo feature like 3d or billboards or something like that. There is a measurable performance hit to the basic example with this change, likely due to the pipeline changes, even though it's not using anything 3d, so I think it would be good to have it be optional in a way that doesn't impact users that don't need it.

Yes this is the right way to do it. No idea how to achieve this however. Also I wonder why there is a performance cost... There should be no entity that requires the 3D pipeline in the basic example. Maybe I should have used the standard render graph pipeline setup.
I won't have any time to work on it until August

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