Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and Improve TerrainPicking #1049

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@MeFisto94
Copy link
Member

commented Mar 19, 2019

by not adding "ghost" collisions to the CollisionResults (which weren't removed).
Improve TerrainPicking by allowing to do more ray tracing (previously, it stopped on the first hit, now it will stop on the first hit within range).

So: I got the report that terrain picking yields collision results even when no collision has happened. This was due to the way the code was structured: The user had to check the result of collideWith to know if a collision happened or not. This is wrong however, because the passed collisionResults struct could already contain results from previous operations (e.g. when picking the scenegraph recursively) and thus the limit check was wrong (it just picked the "closest" collision, which in that scenario makes no sense) and especially the desired result cannot be identified and removed.

During working on that patch I changed semantics: Previously the first hit was returned, even when it's out of range. The code didn't continue to raycast. Now it returns the first hit WITHIN the range or otherwise continues picking. I don't know if that is valid, because I can only imagine situations in which the next raycast is even more out of range. What do you guys think of that situation?

I'm obviously asking you guys to test picking if it has any downsides (the semantics change should at most only waste CPU cycles, I hope).

Depending on the decision about the semantics, I might add another commit which encapsulates the distance check into it's own method to reduce redundancy.

Fix TerrainPicking by not adding "ghost" collisions to the CollisionR…
…esults (which weren't removed).

Improve TerrainPicking by allowing to do more ray tracing (previously, it stopped on the first hit, now it will stop on the first hit within range).
@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

re: "Now it returns the first hit WITHIN the range or otherwise continues picking. I don't know if that is valid, because I can only imagine situations in which the next raycast is even more out of range."

...if the ray is not vertical, and the limit is long, it may collide multiple parts of the terrain, eh? I'm not sure how that matches (or not) with what you've said above. I got confused.

@MeFisto94

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

...if the ray is not vertical, and the limit is long, it may collide multiple parts of the terrain, eh? I'm not sure how that matches (or not) with what you've said above. I got confused.

That's correct, I was under the impression that the multiple parts become further away the more we progress, but that assumption is wrong, to me it looks like it's just checking iteratively for every possible x and y coordinate/patch/what ever, so yeah, it may collide with multiple parts of the terrain.

Which brings up another question: For now it only always ever returned one collision and not multiple. It would be better to have multiple but if we're going to do that, unit tests might be appropriate, to be sure it works.

@stephengold

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

A change like this should include a testcase.

@MeFisto94

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

Good point. The issue happened when the terrain is at a level < y=0 and picking is performed from 0, 0, 0 with a limit < y. Then cr.size() has to be 0 and closestCollision = null.

Maybe we can combine that case with a ray in -1, -1, 0 direction, so it can hit multiple mountains, but that implies generating some terrain somehow (as asset?)

@stephengold

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Existing terrain tests in jme3-examples use the following heightmaps from jme3-testdata:

  • Textures/Terrain/splat/mountains128.png
  • Textures/Terrain/splat/mountains512.png

Other heightmaps available from jme3-testdata:

  • Textures/BumpMapTest/Simple_height.png
  • Textures/Terrain/splat/fortress512.png
  • Textures/Terrain/splat/mountains1024.png
  • Textures/Terrain/splat/pools.png

MeFisto94 added some commits Mar 29, 2019

Upgraded TerrainTestCollision to support multiple collisions and prin…
…t the collision results detailed. MultiCollision can easily be turned on/off in simpleInitApp().

During testing, I noticed a bug where in very rare cases the first collision isn't what is expected but the back side of the clicked mountain. It has to be validated if this is due to the following changes or was already present.
TerrainQuad: Conform with Picker now returning the number of collisio…
…ns and allow to set multipleCollisions true or false.
TerrainPicking: Fixed a bug where the perpendicular collision always …
…returned true, no matter the result of checkTriangles.

Also add support for multiple collisions (which is toggleable to the old behavior, because the picker can early out then).
Fixed a Regression which occurred due to Multi Collision Handling:
The method used to provide duplicate results, which is why I commented it out. This lead to corner-cases not colliding at all anymore, thus I added a unit-test and removed the commented code and instead made addCollision de-duplicate entries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.