In D9969#275650, @Fabian Schempp (fabian_schempp) wrote:

• Takes a single vector which is the position of the mirrored element. Mirror plane will be halfway between origin and position.

I'm not sure how I feel about that. You loose the ability to define arbitrary mirrors, which is a big loss. If the user wanted a quick and easy mirror they would just use the modifier. I just see more users struggling to offset the geometry to fight with the mirror direction. It just feels like the best way to go about it is to define it as if it was setting up an actual planar reflection (position + surface vector), this would be powerful to use alongside vector math nodes.

In D9969#275692, @Kenzie (kenziemac130) wrote:

In D9969#275650, @Fabian Schempp (fabian_schempp) wrote:

• Takes a single vector which is the position of the mirrored element. Mirror plane will be halfway between origin and position.

I'm not sure how I feel about that. You loose the ability to define arbitrary mirrors, which is a big loss. If the user wanted a quick and easy mirror they would just use the modifier. I just see more users struggling to offset the geometry to fight with the mirror direction. It just feels like the best way to go about it is to define it as if it was setting up an actual planar reflection (position + surface vector), this would be powerful to use alongside vector math nodes.

Its actually an arbitrary planar reflection. You can see it in the video.

I sort of agree with Kenzie in that I like the idea of defining the center point of the plane, but I haven't tested this patch so I can't be completely sure.

Something I mentioned in chat: I think one thing it should do is reverse the direction of all polygons in the mesh (by flipping around their section of the corner array). Face normals are calculated with the right hand rule, so it's important to do that or the normals will be wrong on the flipped version.

In general I'd prefer adding new files as C++ files, including the gizmo type file, then you can use types like float4x4 and float3, etc.

This is only some code review. Didn't think about the ui part in more detail yet, Hans has thought about that more than I did :)

I'm not sure I'm a fan of how internally the mirror_mesh function takes a normal input yet the node exposes a rotation. In my opinion rotations should only be used where a unit vector does not provide enough information to an operation, this is not the case here. I can get how Euler might be more friendly to people hand tweaking values it also adds an unnecessary step for people working with vector math who have to convert it to Euler only for it to be converted back internally. Maybe gathering more community feedback could be helpful here.

It also feels like while the widget is cool, since to my knowledge no other node implements widgets and there is no design precedent: it feels a bit "add-onish" rather than an atomic node and might be a bit of bloat that ends up weighing the task down in code review.

In the future more nodes should have their own widgets and it would be awesome to be able for users to write/compose their own for node assets. But for now it feels like you might be taking on some dead weight.

Thank you for working on this!

In D9969#278697, @Kenzie (kenziemac130) wrote:

I'm not sure I'm a fan of how internally the mirror_mesh function takes a normal input yet the node exposes a rotation. In my opinion rotations should only be used where a unit vector does not provide enough information to an operation, this is not the case here. I can get how Euler might be more friendly to people hand tweaking values it also adds an unnecessary step for people working with vector math who have to convert it to Euler only for it to be converted back internally. Maybe gathering more community feedback could be helpful here.

And at the time I decided to use rotation instead of the normal it we haven't had the ball input in Geometry Nodes. It might work a lotbetter with that.

It also feels like while the widget is cool, since to my knowledge no other node implements widgets and there is no design precedent: it feels a bit "add-onish" rather than an atomic node and might be a bit of bloat that ends up weighing the task down in code review.

I still find that without the widget its very hard to understand what is happening. That was the main reason I added it as a possible solution.
But sure, as it is the first node that is using a widget, that puts additional weight on it.

Does this patch still make sense?