New RectangleMesh shape class

[fba-rio]

This RectangleMesh class was created based on the idea proposed at https://hub.jmonkeyengine.org/t/next-release-of-the-engine/44988/170

[stephengold]

This is better. There might be advantages in adding methods to the com.jme3.math.Rectangle class to calculate D and the normal direction vector. What do you think?

[stephengold]

Also ... thinking ahead to how we might test RectangleMesh. One place it might be useful is in example apps (like TestCameraNode, TestTransparentShadow, and TestChaseCamera) that currently use Quad to create a floor for test scenes.

I encourage you to modify these apps (and others) to use RectangleMesh.

[fba-rio]

Yes, a getD() method in Rectangle would be helpful. I'm not sure if the normal direction vector calculation should be there though. Rectangle in the math package essentially is not a mesh. I know that Triangle is not a mesh either, but it makes sense to have a method to calculate the normal vector there because meshes are made of triangles.

Anyway, I am going to open a new PR to add getD() to Rectangle. I can add a method to calculate the normal vector there too if you think it's the right place to have it.

Also, I'll take a look at the examples and modify them to use RectangleMesh.

[stephengold]

Normals (of rectangles) are used to calculate distances and for other purposes that have nothing to do with meshes. So a method to calculate the normal would have broad application, in my opinion.

One tricky thing about normals is documenting how the sign of the normal is determined...

[stephengold]

P.S.: Note that the Rectangle class tends to produce parallelograms if the vertices are not chosen carefully!

[fba-rio]

I moved the calculateNormal() method from RectangleMesh to Rectangle and created a getD() in Rectangle.

I used this PR to make the changes in Rectangle. I hope it is okay.

[stephengold]

It's okay to make both changes in the same PR, since they're so closely related.

However, I think it's unwise to change the semantics of Rectangle as you have done. Other developers may have relied on the old definition of D = (B + C) - A.

[fba-rio]

The thing is if you do the math you won't find the fourth point of the rectangle with that formula. Instead, you will find a point outside the rectangle area.

I don't know where the original developers of Rectangle got that formula from, but clearly there is a mistake there. Anyway, the formula is never used in the source code, it only appears in the javadoc comments. Changing these comments is like fixing a bug, except that I'm not changing the behavior of any function. No source code using Rectangle will be affected. Also, leaving the comments as they are now and calculating getD() differently can be confusing.

[stephengold]

The old formula assumes that the order of the vertices (as you follow the rectangle's perimeter) is A-B-D-C. This isn't the usual labeling used in math textbooks (which favor A-B-C-D). It was careless of Mark and Joshua to use a non-standard vertex ordering without documenting it, but I wouldn't call it a bug.

I'm very reluctant to change the vertex order. After 12+ years of telling developers that D = (B + C) - A, it's very likely someone used that formula, even if it wasn't coded into jme3-core. The best we can do is document the gotchas, including this one.

[fba-rio]

I Understand. Back to the original formula. I documented the order of the vertices in the first javadoc entry of Rectangle.java.

[pspeed42]

Though I still wonder how many of these 'new' shape classes could be replaced by a couple of static methods that operate on any mesh.

[stephengold]

a couple of static methods that operate on any mesh.

Operating on any mesh is a tall order, since JME meshes are extremely flexible: animated/morphing/non-animated, indexed/non-indexed, 9 mesh modes, 9 buffer formats, and all with/without binormals/normals/tangents/UVs/colors, All this flexibility is great for writing importers and generating custom meshes, not so good if you want to manipulate meshes in a general way.

I've been writing mesh-edit utilities for years, but I still don't grok all the options.

So you write a simple utility to translate the vertex positions, but you forget to update the bounds. Or you forget to translate the bind positions of an animated mesh. Then someone finds a mesh with a double-precision position buffer. Or throws interleaved data at it.

[pspeed42]

You know what I meant. Certainly, the dozens of custom shape objects folks are about to make just to offset this or twist that are all going to use the position buffer, normal buffer, etc..

Two or three static methods and a Quad would have been fine. Then you get the added benefit that those static methods would work for box, sphere, <insert any mesh with a Position, Normal, Tangent, etc. buffer here>.

"Write a new shape class every time we want to center a thing" doesn't seem like the right answer.

[stephengold]

While I certainly agree we should be cautious about adding Mesh subclasses to the Engine, this one has an unusually high benefit-to-cost ratio.

While not perfect, I believe this PR is ready to be integrated.

@fba-rio I'd like to document a few caveats and other details. Would it be OK for me to make last-minute changes to the javadoc before integration?

[fba-rio]

@fba-rio I'd like to document a few caveats and other details. Would it be OK for me to make last-minute changes to the javadoc before integration?

Sure, go ahead!

[stephengold]

@fba-rio:

While writing javadoc, I noticed a few issues:

1. If RectangleMesh.setNormal() is used to reverse the normal vectors, the winding order of the triangles will be backward, which might cause unexpected behavior with some materials.

2. The Rectangle.getD() method is inefficient, creating 2 garbage instances of Vector3f each time it's invoked. It would be clearer and more efficient to use:

float x = b.x + c.x - a.x;
float y = b.y + c.y - a.y;
float z = b.z + c.z - a.z;
return new Vector3f(x, y, z);

3. RectangleMesh.updateMesh() should be protected or private, not public.

4. Rectangle.getD() has a misleading name. Perhaps it should be renamed to calculateD().

What do you think?

[fba-rio]

1. If `RectangleMesh.setNormal()` is used to reverse the normal vectors, the winding order of the triangles will be backward, which might cause unexpected behavior with some materials.

Shouldn't the responsibility to provide a valid vector lie with the developer who calls this method? If not, what do you think should be done in this case?

2. The `Rectangle.getD()` method is inefficient, creating 2 garbage instances of `Vector3f` each time it's invoked.

Good point. I made the necessary change.

3. `RectangleMesh.updateMesh()` should be `protected` or `private`, not `public`.

It is protected now.

4. `Rectangle.getD()` has a misleading name. Perhaps it should be renamed to `calculateD()`.

You're right. Renamed.

I think you've made a mistake in the javadoc of Rectangle.calculateNormal. You swapped the (A - B) part of the formula, as seen in the method itself.

[stephengold]

1. If RectangleMesh.setNormal() is used to reverse the normal vectors, the winding order of the triangles will be backward, which might cause unexpected behavior with some materials.

Shouldn't the responsibility to provide a valid vector lie with the developer who calls this method? If not, what do you think should be done in this case?

It's not that simple. For any particular parallelogram, there are 2 plausible normal vectors. For instance, a square in the X-Y plane could have normals of either +Z or -Z. I think the most likely use for setNormal() is to flip the sign of the normals. And flipping the sign is exactly what should reverse the winding order.

I see many possible solutions. One would be to update the winding order in updateMesh() based on the dot product between rectangle.calculateNormal(null) and this.normal, when the latter is non-null. But I think that's overkill. Since there are really only 2 plausible normals for a non-degenerate parallelogram, we just need to provide a mechanism to switch back and forth between them. So I suggest replacing setNormal() with a method named flip() or reverse() or something similar, a method that both negates the normal vector and reverses the winding order.

An even simpler possibility would be to just get rid of setNormal(). In that world, the corners determine the normal. If you want the normal to point in the other direction, you have to shuffle the corners, for instance swapping B and C.

[stephengold]

What does flip() do to the texture coordinates? Is that what a naive user would expect?

[pspeed42]

This is the down side to all-in-one classes versus utility methods. Always one more edge case instead of letting the user just pick the behavior they want.

[fba-rio]

What does flip() do to the texture coordinates? Is that what a naive user would expect?

My bad. I used a seamless texture in my tests and ended up neglecting the texture on flip().

[stephengold]

How would you like to resolve this issue?

[stephengold]

Sorry, I missed 1f20015 for some reason, so ignore my last comment.

I'm still concerned about flip() and its documentation. It appears to re-write the position buffer, which isn't at all what I would expect for an indexed mesh. Also, I expected that flipping a mesh 2x would return the mesh to its original state, but I'm not convinced that's true.

Please describe what flip() does, at a high level, and add that info to the javadoc.

[fba-rio]

I've rewritten the flip() method. Now it doesn't mess with the position buffer anymore, it's restricted to reverse the normal vector direction and rearrange the index buffer (in the updateMesh() method). Also, by doing so it becomes unnecessary to change the texture coordinates.

[stephengold]

That should do it! Thanks for your patience.

[stephengold]

Thanks for your contribution to the project, Francivan! I enjoyed collaborating with you.

[fba-rio]

My pleasure!
And I learned a little bit more in the process.