account for world transform in TerrainQuad.setNormalRecalcNeeded()

[yaRnMcDonuts]

Fixes issue in setNormalRecalcNeeded() method where the bounding box containing recently changed points was always located with the assumption that the terrain's world location is 0,0,0 regardless of the terrain's actual world translation.

[stephengold]

Thanks for your analysis and also for this PR. I have some concerns.

1. This PR addresses cases where the TerrainQuad is translated relative to the world origin, but not cases where it is rotated and/or scaled. Could you address those cases as well?
2. Since the if clause ends with a return, there's no need to follow it with an else.
3. JME's preferred style prohibits the use of tab characters for indentation

[yaRnMcDonuts]

I cleaned up the code as you suggested and also accounted for the terrain's scale.

However I'm a bit stumped as to whether or not the rotation needs accounted for because I'm unable to test it effectively. Unfortunately both the SDK and my own terrain editor glitch out and have the brush stuck at 0,0,0 when a rotated terrain is loaded. It does look like the normals at this spot are updated when I sculpt that center spot of a a rotated terrain, but I can't move the brush away from the center to check if the functionality is okay across the entire terrain.

If someone more experienced multiplying rotations could post the proper line of code, I'll add it in, I assume its a simple line of code but I usually trip over my own code a few times when working with rotations and rely on trial/error to make sure its working, but in this case I'd feel uncomfortable merging in code that I can't extensively test right away.

[stephengold]

Unfortunately, that scaling code is incorrect because it scales the world position instead of the changed point.

If handling rotations accurately is very difficult (and I suspect it is) then it might be better to bail out when setNormalRecalcNeeded() is applied to rotated TerrainQuad---either throw an exception or grow the affected area to cover the entire quad, whatever makes sense in this context.

If the X, Y, and Z components of the world rotation are all zero, you can be sure the quad isn't rotated.

[yaRnMcDonuts]

My mistake, I fixed the scaling code to work properly now, and this time tested it with an offset scale and location at the same time to ensure it's working. Let me know if there's anything else that looks like it can be improved.

If handling rotations accurately is very difficult (and I suspect it is) then it might be better to bail out when setNormalRecalcNeeded() is applied to rotated TerrainQuad---either throw an exception or grow the affected area to cover the entire quad, whatever makes sense in this context.

Yes that sounds like a good idea, I think growing the affected area to cover the entire quad is a good solution. It will also be very rare that someone wants to edit a rotated terrain in the first place, because then they'd have to worry about handling rotations when they call the setHeight(Vector2f vertPos) method that requires local, non rotated coordinates. This is why I made my terrain editor able to sculpt scaled but not rotated terrains, and the SDK's editor appears to work the same.

[stephengold]

The cast will throw an exception if the world bound is spherical. One way to handle this case would be to convert the sphere to a box like so:

    BoundingVolume bv = getWorldBound();
    if (bv instanceof BoundingSphere) {
        BoundingSphere bs = (BoundingSphere) bv;
        float r = bs.getRadius();
        float center = bs.getCenter();
        affectedAreaBBox = new BoundingBox(center, r, r, r);
    } else {
        affectedAreaBBox = (BoundingBox) bv.clone();
    }

Once the affectedAreaBBox encloses the world bound, is there any reason to keep merging points into it? If not, then the world rotation should be tested before the worldLocVec2 calculations and should cause an early return from the method.

Is there a good reason to use getX() and getZ()? The fields of Vector3f are public, so .getX() can be replaced by .x and .getZ() can be replaced by .z.

Comparing the world rotation to Quaternion.IDENTITY isn't the best way to test for null rotation, because it treats (0, 0, 0, -1) (which is a null rotation) as a non-null rotation. And unfortunately, Quaternion.isIdentity() has the same flaw. My recommendation is still:

    Quaternion wr = getWorldRotation();
    if (wr.getX() != 0 || wr.getY() != 0 || wr.getZ() != 0) {

[yaRnMcDonuts]

I just went with the getX() and getZ() methods because I thought it was considered better practice to always call getters and setters as opposed to using .x and .z references, but I'm also not up to date with best practices since most of my coding work is on personal projects where I can get away with small things so long as the performance impact isn't big. I've changed it to .x and .y as you suggested.

I also added the code for a bounding sphere as you suggested. I initially didn't think It would be necessary because I think using a bounding sphere for its bounds would break a terrain's collisions if its too small or would deoptimizes the terrain's framerate if its too big. But I'm just speculating so I added the code you provided so it will function if anyone ever needs to do things with a spherical world bound for some reason.

You are also correct that there's no reason to continue merging points if the terrain is rotated so I added a return at the end of the if check for a rotated terrain.

Edit: it looks like I made a mistake that caused the build to fail, it is late so I will fix it when I am more alert. I'm still running and testing my changes on 3.3. rather than master so instead of uploading the whole TerrainQuad file from my repo, I've been editing single lines in the browser editor which makes me more prone to mistakes, and I think that was also causing me the tab indentation issues earlier.

[stephengold]

Thanks for your contribution, Ryan.