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] snap picking #1595

Merged
merged 2 commits into from
Jul 26, 2024
Merged

[FIX] snap picking #1595

merged 2 commits into from
Jul 26, 2024

Conversation

Kurtil
Copy link
Contributor

@Kurtil Kurtil commented Jul 25, 2024

This PR fixes two issues:

  • it was possible to pick unpickable objects (pickable: false) when using snap picking on DTX scene models.
  • VBO vertex snap picking was broken if a unpickable object was present on the model. The rest of the comment will focus on this second issue.

To reproduce, go on a snap to vertex example, get one model entity and set its pickable property to false.
-> The snap cursor is stuck somewhere around the origin of the model.

On this image, my mouse is hovering the model and the pink top element of the table is unpickable. The green circle is positioned on the snappedCanvasPos that is stuck:
Screenshot 2024-07-26 at 00 35 34

This issue was only present on VBO and only on snap to vertex picking.
It was not present in DTX because of the previous issue AND vertex are culled using gl_Position = vec4(3, 3, 3, 1);)

What may happen:

Unpickable vertex are culled using gl_Position = vec4(0); here (idem for instancing and lines)
Because of how the snap picking works, it may not be a good idea to cull a vertex this way.

I saw this formula here:

Screenshot 2024-07-26 at 01 07 15

Vertex with a vec4(0) position ends into the NDC rectangle [-1, -1, -1] to [1, 1, 1] as its position is [0, 0, 0] and because position.w is 0, it is not clipped/discarded as 0 <= 0 <= 0 for each x, y, z components.

Vertex snap picking uses point primitive. I assume "culling" a triangle with the same vec4(0) position for the three corner vertices may work because the triangle is too small to be rendered... but a point with pointSize of 1 may still be rendered.

Because of that, unpickable objects vertices are always rendered as point of size 1 at the center of the snapping viewport.
Then, due to this line, the returned snappedCanvasPos seems to be always the origin of the model. Indeed, [0, 0, 0] relative to origin is the origin... ?

I hope it makes sense. Even if what I described here is not 100% correct, moving the vertices at vec4(2, 0, 0, 0) seems to fix the issue. I think any value other that 0 may work du to the previous formula.

@Kurtil Kurtil changed the title Fix/snap picking [FIX] snap picking Jul 25, 2024
@Kurtil
Copy link
Contributor Author

Kurtil commented Jul 25, 2024

vbo_instancing_autocompressed_triangles.html.zip

The vbo_instancing_autocompressed_triangles example updated with snap to vertex picking to reproduce the second issue.

@Kurtil
Copy link
Contributor Author

Kurtil commented Jul 26, 2024

What makes no sens however is the perspective division by 0 after the vertex passing the clipping test…

@Kurtil
Copy link
Contributor Author

Kurtil commented Jul 26, 2024

What makes no sens however is the perspective division by 0 after the vertex passing the clipping test…

I tested on a simple example, drawing only one Point primitive at the center of the screen with position.w = 0 and it works! Line and Triangle primitives however are not displayed if position.w = 0. It may be a a specific case for the Point, handling the perspective division by zero leading to undefined or something that does not discard the point... 🤔 Platform : macOS Sonoma 14.5, on chrome, firefox, and safari

@xeolabs xeolabs merged commit e0eeae8 into xeokit:master Jul 26, 2024
2 checks passed
@xeolabs
Copy link
Member

xeolabs commented Jul 26, 2024

All makes sense to me, thanks!

@Kurtil Kurtil deleted the fix/snapPicking branch July 26, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants