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

Improve picking color generation #2697

Merged
merged 3 commits into from
Feb 19, 2019
Merged

Improve picking color generation #2697

merged 3 commits into from
Feb 19, 2019

Conversation

Pessimistress
Copy link
Collaborator

Calculate picking color for 1M instances, before: 45 iterations/s; after: 412 iterations/s

Since layer.calculateInstancePickingColors always produces the same sequence, we can save the largest generated array and copy its value into others.

We can potentially make all instancePickingColors attributes share the same WebGLBuffer, but that would introduce more complexity around:

  • Retrieving the shared buffer by GL context
  • Creating a temporary buffer when performing multi-depth picking

Change List

  • Optimize layer.calculateInstancePickingColors
  • Benchmark case

Copy link
Contributor

@tsherif tsherif left a comment

Choose a reason for hiding this comment

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

Main concern here is the internal cache pointing the the value arrays in the attributes it gets as arguments. Seems like a data-corruption risk, since it's unclear who owns the array. Since the values are always the same, would it make sense to generate the pick values array externally, and send it to the pick color attributes? I.e. make that an explicit part of the attributes API.

modules/core/src/lib/layer.js Outdated Show resolved Hide resolved
modules/core/src/lib/layer.js Outdated Show resolved Hide resolved
@Pessimistress Pessimistress merged commit 2c15c55 into master Feb 19, 2019
@Pessimistress Pessimistress deleted the picking-perf branch February 19, 2019 21:46
Pessimistress added a commit that referenced this pull request Feb 20, 2019
Pessimistress added a commit that referenced this pull request Feb 22, 2019
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