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

feature: add dual snap mode => vertex + edge #1158

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Conversation

Kurtil
Copy link
Contributor

@Kurtil Kurtil commented Oct 4, 2023

This PR adds the ability to snap vertex AND edge. If both are found, vertex are snapped with higher priority. The main purpose of this changes is to do this dual snap picking with only one depth buffer initialisation to improve performance in comparaison of doing two snap picking simultaneously with different snap mode.

dualSnap

API changes : (BEAKING CHANGES)

  • snapMode is only used as frameCtx property.
  • CameraControl now has snapVertex: boolean & snapEdge: boolean properties instead of snapMode. (In case of both are false, the snapPick falls back to a regular pick with canvasPos and pickSurface: true)
  • scene.snapPick accept snapVertex: boolean & snapEdge: boolean arguments instead of snapMode.
  • pickResult has snapType: null|"vertex"|"edge" property instead of snappedToVertex: boolean & snappedToEdge: boolean.

Implementation :

Only Renderer logic, no shader update. No changes for mono type snap (only vertex or only edge).

On dual type snap, frameCtx.snapPickLayerParams stores empty x 1 then the edge layer params first, then empty x 1, then the vertex layer params. The empty x 1 is because the w of the drawn snap color is 0 if no snap, and if !== 0, represents the layer params number. The second empty x 1 is to make this line works fine (done by the frameCtx.snapPickLayerNumber++ between the two snapPickDrawSnapDepths calls):

const pickedLayerParmasSurface = layerParamsSurface[Math.abs(pickResultMiddleXY[3]) % layerParamsSurface.length];

Indeed, if layerParamsSurface has a length of 10, pickResultMiddleXY[3] can be 12 if a vertex snap was drawn but thanks to the % it falls back to the 2 index and layerParamsSurface[foundIndex] is not undefined.

Then, this line allow to know if the snap pick is a vertex pick or an edge pick:

const isVertex = snapVertex && snapEdge ? snapPickResultArray[i + 3] > layerParamsSnap.length / 2 : snapVertex,

Has edge pick are stored on the first half of the layerParamsSnap array and vertex pick on the last half.

To go further

In case of a snapPick in center position, the previous implementation got the pickedLayerParmasSurface from the layerParamsSurface (see here) array which is not correct as snap pick params are stored in the layerParamsSnap array. But if this is fine, the layerParamsSnap may not be really needed as it is the same values as the layerParamsSurface array. It may be interesting to simplify this code to only use the layerParamsSurface array.

@ghost
Copy link

ghost commented Oct 4, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@xeolabs xeolabs added this to the 2.4.0 milestone Oct 4, 2023
@xeolabs xeolabs added the enhancement New feature or request label Oct 4, 2023
@xeolabs xeolabs merged commit 7ab6b46 into xeokit:master Oct 4, 2023
@xeolabs
Copy link
Member

xeolabs commented Oct 4, 2023

Makes perfect sense, thanks @Kurtil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants