Skip to content

Hexasphere integration#1944

Closed
OptimisticPeach wants to merge 13 commits intobevyengine:mainfrom
OptimisticPeach:hexasphere_integration
Closed

Hexasphere integration#1944
OptimisticPeach wants to merge 13 commits intobevyengine:mainfrom
OptimisticPeach:hexasphere_integration

Conversation

@OptimisticPeach
Copy link
Contributor

This integrates the hexasphere code as discussed in #1595.

This is a draft PR because I have yet to implement any other prebuilt shapes, such as a plane, or a UV sphere.
Additionally, it is lacking documentation, which will be added prior to the PR being merged.

Please let me know what you think about the API though:

  • Mesh::subdivide<I: Interpolator>(&mut self, subdivisions: usize, interpolator: I)
  • bevy_render::shape::shapegen::subdivide<I: Interpolator>(mesh: &mut Mesh, subdivisions: usize, interpolator: I)

There are multiple interpolators available:

  • IdentityInterpolator - Returns LHS.
  • LinearInterpolator
  • NormalizedLinearInterpolator
  • SphericalInterpolator.

Individually, these all implement AttributeInterpolator<A>, as long as A make sense for them.

Interpolators are grouped together under a type which implements Interpolator, after which we then let the interpolator group (I've suffixed these types with -Group) choose the adequate interpolator based on the name and type of attribute.

My apologies for the monstrous size of this PR!


Additionally, there was an issue when committing my changes on the cargo.toml not sure what happened there.

Changes made:
- Swap Y/Z when calculating UV coordinates
- Correct mapping in the UV coordinates
- Fix type in Azimuth
It compiles, and the subdivision code is there.
Removes dependency on hexasphere
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on labels Apr 17, 2021
@alice-i-cecile
Copy link
Member

@aevyrie, would you like to be added as a reviewer to this? It touches on your geometry work quite closely.

@aevyrie
Copy link
Member

aevyrie commented Apr 17, 2021

Sure, just be aware I'm very time limited at the moment, so it could be delayed.

@alice-i-cecile alice-i-cecile requested a review from aevyrie April 17, 2021 04:37
fn float(&mut self, _: &str) -> &mut LinearInterpolator {
&mut self.lerp
}
fn float2(&mut self, _: &str) -> &mut SphericalInterpolator {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this could be incorrect: it would spherically interpolate 2D attributes which doesn't make sense for something like UV coordinates. I believe that changing this to be a linear interpolator would make more sense.

@aevyrie
Copy link
Member

aevyrie commented Apr 20, 2021

I looked through this PR, though most of it is lost on me without comments. I'd be happy to take a closer look when you get to that stage. 🙂

I've collected some questions I had after reading your post and perusing the draft PR. I understand that the intent is to provide subdivision methods for spheres and planar surfaces?

Mesh::subdivide<I: Interpolator>(&mut self, subdivisions: usize, interpolator: I)
bevy_render::shape::shapegen::subdivide<I: Interpolator>(mesh: &mut Mesh, subdivisions: usize, interpolator: I)

I like this, but I'm curious how this will work for non-primitive meshes? E.g. what happens when I load up a gltf, access the mesh, and .subdivide()?

@OptimisticPeach
Copy link
Contributor Author

I looked through this PR, though most of it is lost on me without comments. I'd be happy to take a closer look when you get to that stage. 🙂

It's a lot of code to document, and I'd rather get the actual code down before I start to write documentation (given how tedious it can be to document code well). I think it might be beneficial to document the public API however, since that's what I really want comments on.

I understand that the intent is to provide subdivision methods for spheres and planar surfaces?

I originally designed it to work for spheres, however I later realized that it could be extended to any arbitrary mesh of triangles. It doesn't support more interesting subdivision however, as though interpolation is done only over two vertices and a percent value. Would adding that capability be of value?

I like this, but I'm curious how this will work for non-primitive meshes? E.g. what happens when I load up a gltf, access the mesh, and .subdivide()?

Here's what the subdivide function will do:

  • Ensure the mesh has indices. If not it'll generate them (0, 1, 2, 3, 4, etc. assuming the indices are in order).
  • Find the attribute with the shortest number of elements (should we make it configurable whether it's shortest, longest, or custom?), and then find all attributes with the same number of elements (since attribute sizes are independent of each other).
  • Group indices into "triangles", trying to efficiently share edges if there were provided indices.
  • Subdivide the "triangles" by lazily adding default attributes to the end of the list. This is done by the Attributes struct.
  • Actually add the attributes with their default values.
  • Calculate their values, attribute by attribute and then index by index, as opposed to index by index and then attributes. This should theoretically improve cache times.
  • Since the indices of the subdivided elements are stored in structs which represent the original triangles we grouped the mesh into, we need to iterate through those and extract indices for the inner triangles. This is done in add_indices_triangular, which deals with logically adding the indices to keep winding. This was worked out on paper and I doubt I'll be able to adequately document this function.
  • Set the new indices in the original mesh.
  • End of function.

So we're just modifying the original mesh, meaning that if someone wants to create a different one, they have to copy the mesh beforehand.

@aevyrie
Copy link
Member

aevyrie commented Apr 20, 2021

It's a lot of code to document, and I'd rather get the actual code down before I start to write documentation (given how tedious it can be to document code well). I think it might be beneficial to document the public API however, since that's what I really want comments on.

I'm sure the public API docs you mention will cover this. I also understand this is still a draft. 🙂

I originally designed it to work for spheres, however I later realized that it could be extended to any arbitrary mesh of triangles. It doesn't support more interesting subdivision however, as though interpolation is done only over two vertices and a percent value. Would adding that capability be of value?

Could you clarify what the "Sphere" in SphereInterpolator refers to? Is it using vertex normals to determine the new vertex position assuming the surface is spherical (as opposed to planar)? When first writing this reply I thought it was only useful for subdividing spheres but I realize I misunderstood.

I'm also thinking about the implications for 3d primitives. How do we prevent users from grabbing this subdivide function, not realizing it isn't necessarily going to give them an accurate result? The results are probably going to be fine in most cases, but I think this API "misuse" is worth considering.

@OptimisticPeach
Copy link
Contributor Author

Could you clarify what the "Sphere" in SphereInterpolator refers to?

It refers to spherical interpolation: https://en.wikipedia.org/wiki/Slerp#Geometric_Slerp, which will interpolate around a unit circle/sphere/hypersphere.

Is it using vertex normals to determine the new vertex position assuming the surface is spherical?

No, it is doing no such thing (since we can't assume the mesh will have vertex normals). Instead it interpolates positions only. Adding access to other attributes while interpolating might be something to explore though.

When first writing this reply I thought it was only useful for subdividing spheres but I realize I misunderstood.

Well, SphereInterpolator is only useful for dividing subdividing spheres since it'll only do unit spheres properly.

I'm also thinking about the implications for 3d primitives. How do we prevent users from grabbing this subdivide function, not realizing it isn't necessarily going to give them an accurate result?

This is a good point; most people would expect something like a blender subdivide, whereas this essentially offers static LOD. Doing that however, would require more interpolation parameters to do better spline-style interpolation. Is this something we would want? Additionally an issue with this is in meshes without indices, which makes it incredibly difficult to figure out edges to work with (and we can't implement a HashMap/Set based solution since f32, f64 and arrays of such aren't Hash).

@aevyrie
Copy link
Member

aevyrie commented Apr 22, 2021

Thanks for clarifying!

Maybe it would make more sense to build this off of 3d primitives, so we know exactly what type is being meshed and subdivided? That way the subdivide function can be implemented per-primitive. That might simplify some of the other aspects of this API.

Documented a few items, still missing a few more though.
It compiles, and the subdivision code is there.
Removes dependency on hexasphere
Documented a few items, still missing a few more though.
Also fixes the PR; this hadn't been updated in a while.
@OptimisticPeach OptimisticPeach deleted the hexasphere_integration branch July 4, 2021 21:46
@OptimisticPeach
Copy link
Contributor Author

I accidentally deleted the branch while trying to get new commits onto it. I'm going to finish making the commits I intended on doing before I open a new PR though.

Specifically, I'm going to add code for generating the new shapes as described in the geometric primitives RFC along with documenting them. I'll probably give them APIs adjacent to the current IcoSphere, and how they interact with the other PR can be decided later (after the geometric primitives PR is merged, and the new PR for hexasphere_integration is being reviewed).

Sorry for the confusion!

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-Dependencies A change to the crates that Bevy depends on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants