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

attribute_multi_mut to allow simultaneous mutable access to distinct mesh attributes #2862

Closed
wants to merge 4 commits into from

Conversation

SorteKanin
Copy link

Objective

Fixes #2134, allowing simultaneous mutable access to distinct mesh attributes.

Solution

The attribute_multi_mut method allows the user of the API to retrieve a constant amount of mutable references to unique attributes. The method uses constant generics for a clean interface. The method uses some unsafe code to retrieve multiple mutable pointers to the given vertex attributes, then verifying that they are indeed all unique (and panics otherwise).

I've also added some docs to explain the method and some tests to ensure that it works as expected.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 23, 2021
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen S-Needs-Review C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Sep 23, 2021
/// Returns None at index i if the mesh had no data for the vertex attribute given at index i.
///
/// # Panics
/// Panics if any of the given attribute names resolve to the same attribute, as this would otherwise allow aliased mutability.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super familiar with the rendering side of things: I take it there's a reason we can't just supply a HashSet? Generally I prefer type-enforced rather than panic-enforced rules of this sort.

At first glance I'm guessing this is impossible because a) performance costs (arrays are GPU-friendly) and b) it looks like you can have synonyms that point to the same data.

Copy link
Author

@SorteKanin SorteKanin Sep 23, 2021

Choose a reason for hiding this comment

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

It's not impossible and I did consider a HashSet. However I think the const array approach provides a nicer interface as you can immediately destructure the array into the same amount of attribute you provided. I think this is much more readable than getting a HashSet and then having to get keys out of this.

Besides, I think most use cases of this function will be just a static array where the distinctness of the attributes is obvious, and a HashSet would just be a tedious intermediary. I think users would often just assume the keys exist and panic themselves anyway.

The better performance is just gravy on top :)


assert_ne!(
current, *other,
"Duplicate names given to attribute_multi_mut."
Copy link
Member

Choose a reason for hiding this comment

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

It sounds like the problem isn't duplicate names, it's that the names provided point to the same attributes. Otherwise we could just check this at the top.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I understand - how can the names be different but point to the same attribute?

Copy link
Member

Choose a reason for hiding this comment

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

My bad, I wasn't sure if there was the possibility of synonymous names in a rendering context.

Why not just check string equality immediately?

Copy link
Author

Choose a reason for hiding this comment

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

I'd have to gather up all the strings (calling .into()) ahead of time and then compare. I suppose it could work just as well but comparing the pointers seemed to more explicitly state the requirement of no aliased mutability.

Also its just faster :)

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, I'm happy with this.


// Verify that there are no duplicate pointers.
// This is critical to ensure no aliased mutability!
for i in 1..pointers.len() {
Copy link
Member

Choose a reason for hiding this comment

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

This approach to duplicate checking will be O(n^2). Can we use Hash to get an O(n) solution here instead? Do we even care? My intuition suggests that n will be very small here.

Copy link
Author

Choose a reason for hiding this comment

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

We could use a temporary HashSet here, yes but as you alude to, I suspect the heap allocation of that set would be more expensive than the O(n^2) here, where n is probably very rarely more than 5.

Copy link
Author

Choose a reason for hiding this comment

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

Added a comment to make this explicit.

) -> [Option<&mut VertexAttributeValues>; N] {
// SAFE: We verify that all the references returned are unique,
// therefore no aliased mutability is possible.
unsafe {
Copy link
Member

@alice-i-cecile alice-i-cecile Sep 23, 2021

Choose a reason for hiding this comment

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

Try to make your unsafe blocks as small as possible; it makes functions like this easier to review and change.

Copy link
Author

Choose a reason for hiding this comment

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

The unsafe block would be very small indeed for this function if I reduced it as much as possible - I think there is only one line that strictly requires it. I personally think it makes sense to also enclose the surrounding code that ensures the safety of the "unsafe required" code.

But I can reduce the unsafe block's size if you insist :)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's a sensible point. I think I would personally use a small unsafe block with a safety comments calling out how the rest of the function ensures that it's safe, but that's something I'm happy to punt on to more senior team members.

Copy link
Author

Choose a reason for hiding this comment

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

I thought about it and I don't mind just that one line being the unsafe block :)

Copy link
Contributor

@inodentry inodentry left a comment

Choose a reason for hiding this comment

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

This seems very useful, and the implementation looks good to me. :)

@inodentry inodentry added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Sep 24, 2021
// Just setting them equal to each other for the heck of it.
normals[0] = [0.1, 0.2, 0.3];
positions[0] = normals[0];
assert_eq!(normals[0], positions[0])
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this assert is doing anything. If it failed, it would have failed to compile before.

Would it make more sense to check on the attribute value by getting them from the mesh with something like

assert_eq!(mesh.attribute(Mesh::ATTRIBUTE_POSITION).unwrap()[0], mesh.attribute(Mesh::ATTRIBUTE_NORMAL).unwrap()[0]);

Copy link
Author

Choose a reason for hiding this comment

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

I suppose you're right that it doesn't do anything. I've updated to something like your version :)

@cart
Copy link
Member

cart commented Feb 3, 2022

Before committing to this, I think its worth considering a simpler alternative (that removes the need for dupe checks at the cost of one extra btree lookup):

let mut mesh = Mesh::from(shape::Box::default());

let positions = mesh.remove_attribute(Mesh::ATTRIBUTE_POSITION).unwrap();
let normals = mesh.remove_attribute(Mesh::ATTRIBUTE_NORMAL).unwrap();

// modify here

mesh.insert_attribute(Mesh::ATTRIBUTE_POSITION, positions);
mesh.insert_attribute(Mesh::ATTRIBUTE_NORMAL, normals);

@SorteKanin
Copy link
Author

@cart That could definitely work too. I'm not sure which method is faster, but it probably doesn't matter too much either way.

I'm more concerned about the potential for bugs with the remove_attribute method. It's much easier to make a mistake with that method and, for example, forget to re-insert the attribute, or re-insert it under a wrong name. Not a huge worry but still.

But if the remove_attribute method is more generally useful and would be added anyway, then that's a good enough workaround and this maybe isn't needed. Then again I don't see a lot of utility in removing mesh attributes (when would you want to do that?) but maybe I'm ignorant.

@cart
Copy link
Member

cart commented Feb 3, 2022

I'm more concerned about the potential for bugs with the remove_attribute method. It's much easier to make a mistake with that method and, for example, forget to re-insert the attribute, or re-insert it under a wrong name. Not a huge worry but still.

Yup I agree that this does introduce a level of "logical error potential". But it also removes the need for some gnarly unsafe code and keeps the implementation small and simple. Given that the need for "multi-mut" access is probably pretty niche, I think I'm comfortable with that tradeoff.

But if the remove_attribute method is more generally useful and would be added anyway, then that's a good enough workaround and this maybe isn't needed. Then again I don't see a lot of utility in removing mesh attributes (when would you want to do that?) but maybe I'm ignorant.

In addition to enabling multiple mutable accesses, it also gives developers the ability to remove attributes they don't need. For large meshes, this could free up significant amounts of memory.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label May 16, 2022
@komadori
Copy link
Contributor

Without prejudice to this PR, Mesh::remove_attribute() was added by #5254.

@cart
Copy link
Member

cart commented Sep 27, 2022

Closing as I think remove() is preferable to adding unsafe code for this use case, which is likely pretty niche. If there is enough demand, we can revisit this.

@cart cart closed this Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simultaneous mutable/immutable access to different mesh attributes
6 participants