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(layers): Fix MVTLayer + pickMultipleObjects #9246

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

donmccurdy
Copy link
Collaborator

The instancePickingColors attribute has some hard-coded assumptions about size, such as...

attributeManager.addInstanced({
instancePickingColors: {
type: 'uint8',
size: 4,

... but MVTLayer currently initializes the buffer with 3, not 4, components. This leads to a bug in pickMultipleObjects where disablePickingIndex fails to disable the previously selected object index (or goes out of bounds) and the operation just returns depth duplicate references to the top hit.

While we could try to handle multiple attribute sizes, I think it's probably preferable to just have one way to represent things. Here I've switched MVTLayer to 4-component picking colors, following WebGL's 4-byte alignment best practices.

Change List

  • fix(layers): GeoJSONLayer and MVTLayer use 4-component instance picking colors

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file can be reverted before submitting the PR — they're not required for the fix — but I wanted to highlight one place where the 3 vs 4 component issue came up. Here colors had one size and externalColorAttribute had another, to the indices didn't map correctly.

If we did prefer to support both 3-component and 4-component instance picking colors, though, this isn't enough, there'd be at least 1-2 more places requiring similar fixes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel regardless of how we address it, the fix should go in the Attribute class:

  • getVertexOffset should use the "real size"
  • there should be an alignment check when size is overridden by an external binary attribute. We moved to 4-component picking color because WebGPU will not support 3-byte colors.

Since these are not required to patch this bug, they should be made against master and the next minor release?

Copy link
Collaborator Author

@donmccurdy donmccurdy Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - for now I've reverted the change to layer.ts, so only the change from 3-component to 4-component picking colors is included in the PR.

@coveralls
Copy link

Coverage Status

coverage: 91.626%. remained the same
when pulling 2f8004b on donmccurdy/fix-mvtlayer-pick-multiple
into 27d2010 on master.

@donmccurdy donmccurdy merged commit 81371bf into master Nov 14, 2024
4 checks passed
@donmccurdy donmccurdy deleted the donmccurdy/fix-mvtlayer-pick-multiple branch November 14, 2024 18:03
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.

[Bug] PickMultipleObjects Returning the Same Objects Repeated With MVT Layer
3 participants