komadori / bevy_mod_outline

Apache License 2.0
123 stars 10 forks source link

Undesirable results for "flat" meshes #12

Closed mxgrey closed 1 year ago

mxgrey commented 1 year ago

I've noticed that meshes which have "flat" regions have some issues with being highlighted correctly.

Here's a gist to illustrate the problem. The code there should be compatible with the latest master of bevy_mod_outline.

Here's a screenshot to give a rough idea of the problem: outline_v3

The cyan shape is "flat" meaning it has infinitely sharp edges. The outlining technique used by bevy_mod_outline doesn't have any good choice of direction to move the outline vertices, so certain edges of the outline will get cut off depending on the angle of the camera relative to the mesh face.

I don't expect bevy_mod_outline to solve this problem completely because a flat mesh doesn't generally provide enough information to reliably infer how to construct the outline mesh. I believe getting a good outline for the flat shape would require adding extra vertices and elements to the outline mesh, and that's probably beyond the scope of what bevy_mod_outline can/should be doing.

However, I think this adjustment to bevy_mod_outline could allow users to fix these edge cases themselves while still taking advantage of bevy_mod_outline:

Users can then customize the outline mesh however they need to for the effect that they want. For the flat mesh, extra vertices can be added at each corner with normal vectors that go out in all the necessary directions, and those new vertices can be connected together with new index elements. The rest of the pipeline, including the stenciling, can remain the same.

There are probably other edge cases that this adjustment would help to solve as well.

Does this sound like a desirable change to the crate? Since OutlineMesh would be an optional component, I believe the change could be made without breaking the API.

komadori commented 1 year ago

Thanks for bringing this up. This idea has occurred to me and I called the proposed component OutlineMesh in my head too. However, it's quite a blunt feature and I'm not sure that it's the best way to add value.

Firstly, I assume you're aware of ATTRIBUTE_OUTLINE_NORMAL. In the case of flat polygons like the one shown in your screenshot, it is possible to calculate outlining normals that would not result in the outline being partially occluded. Although, i) the library doesn't offer such a calculator at present and ii) as with any hard edges the line thickness becomes inconsistent to some degree. auto_generate_outline_normals assumes the object is closed and has sensible normals already.

So, as you say more or less, to improve the quality of outlining hard edges you need to add extra triangles connecting surfaces that face in different directions. These triangles would be degenerate in the original object, but expand to fill the gaps when rendering the outline. You can author these into a mesh today, but you will pay the (small?) cost of the degenerate triangles when rendering the original object.

I see OutlineMesh as on one hand a performance improvement to only include the degenerate triangles when outlining, but on the other hand a waste of memory to be duplicating the rest of the mesh. I'm interested in whether I could i) calculate the degenerate triangles needed from a mesh automatically and ii) how to efficiently store the mesh so that I can render it with or without them as needed.

My question to you is then, do you really feel the need to have outline and non-outline versions of each mesh? If so, what will that accomplish versus building outline normals and/or extra triangles into a single mesh? Is it performance?

mxgrey commented 1 year ago

do you really feel the need to have outline and non-outline versions of each mesh? If so, what will that accomplish versus building outline normals and/or extra triangles into a single mesh?

This is a completely fair point... You're right, since the extra triangles would consist of perfectly overlapping vertices or perfectly flat lines, they would have zero area/volume and should not even be visible despite existing on the original mesh. I hadn't really considered this because it felt like "bad hygiene" for the mesh to contain vertices and elements that aren't supposed to be rendered, but I can't think of any real problem with it besides feeling a little icky. Maybe it creates a little more work for the fragment shader, but that's probably a negligible concern compared to storing a whole second mesh in memory.

I do still think there should be a unit component for blocking the auto outline normal generation for specific entities. I'd like to have that feature enabled for most meshes but not the ones where I'm specifically customizing the outline normal attribute.

mxgrey commented 1 year ago

I do imagine there could still be a use case for an OutlineMesh component if someone wants to artistically customize their outline in a way that would require them to have elements with non-zero area in the outline mesh. But my use case doesn't require that, so I suppose it's fair to kick that down the road until someone actually needs it.

komadori commented 1 year ago

I do imagine there could still be a use case for an OutlineMesh component if someone wants to artistically customize their outline in a way that would require them to have elements with non-zero area in the outline mesh. But my use case doesn't require that, so I suppose it's fair to kick that down the road until someone actually needs it.

I don't disagree. I guess my own intuition was that it was "bad hygiene" to duplicate the mesh just to add some more triangles. However, maybe this may end up being the best solution. I was wondering is something like... having a separate index buffer for additional triangles which shared the main mesh's attribute buffer would be nicer.

I do still think there should be a unit component for blocking the auto outline normal generation for specific entities. I'd like to have that feature enabled for most meshes but not the ones where I'm specifically customizing the outline normal attribute.

You're using AutoGenerateOutlineNormalsPlugin? Really, I envisaged this as quick way to help people whose "outlines are broken" and that more advanced users would call generate_outline_normals specifically on the meshes that need it, or have outline normals saved in their asset files (see bevy_mod_gltf_patched and https://github.com/bevyengine/bevy/pull/5370).

The auto-generator plugin hooks AssetEvent to process Meshes so it can't do anything entity specific. One related idea that does occur to me is that I could add ATTRIBUTE_OUTLINE_NORMAL_AUTO as a third option for outline normals with a precedence between the existing two. The auto-generator would create this attribute instead of ATTRIBUTE_OUTLINE_NORMAL so that custom outline normals would never be overridden or overwritten. Would that be helpful to you?

mxgrey commented 1 year ago

Ah good point about meshes not being owned by a single entity. That does make the component filtering approach ineffective. Having an ATTRIBUTE_OUTLINE_NORMAL_AUTO is an interesting idea.

You're using AutoGenerateOutlineNormalsPlugin?

In my application there are meshes being loaded from arbitrary sources, so I figured auto-processing all the normals would be a better idea than not. But I suppose it wouldn't be much work to remove that plugin and be more selective in my application logic about which meshes are getting processed.

mxgrey commented 1 year ago

I've created a new gist of the previous example but with a function that creates outline normals along the edges in the way we've discussed. I think the result is pretty nice, and the function I came up with is fairly general for closed-loop paths, so others can feel free to copy/paste it.

I'll close this issue since I think the changes I requested aren't necessary for the motivating problem.

komadori commented 1 year ago

FWIW, I've been leaning towards just adding OutlineMesh anyway.

@mxgrey I haven't had the time to properly examine your function yet, but it looks interesting. For future reference, is it licensed under the Apache/MIT arrangement suitable for inclusion into bevy_mod_outline?

mxgrey commented 1 year ago

Yes, please consider that gist to be licensed under Apache + MIT, and feel free to include it in bevy_mod_outline. I would open a PR myself but it's not clear to me how to make it ergonomic for general purpose use. For example, with some small tweaks the same function would also be suitable for producing an outline of an open line path rather than a closed loop path.