Skip to content

Expose the pick state and use pixel position from params#36

Merged
hccampos merged 2 commits intopnext:masterfrom
Zielon:feature/pixel_pos_from_params_pick
Apr 2, 2019
Merged

Expose the pick state and use pixel position from params#36
hccampos merged 2 commits intopnext:masterfrom
Zielon:feature/pixel_pos_from_params_pick

Conversation

@Zielon
Copy link
Copy Markdown
Contributor

@Zielon Zielon commented Mar 27, 2019

In the case when we want to provide different coordinates for the picking window, the property from params, pixelPos can be used.

Copy link
Copy Markdown
Contributor

@hccampos hccampos left a comment

Choose a reason for hiding this comment

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

Thanks for the feature. Could be useful for some people indeed. Just don't agree with exposing internal details of the picking.

Comment thread src/point-cloud-octree.ts Outdated
}

interface IPickState {
export interface IPickState {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why should this be exported? It is an internal detail of the picking system and shouldn't be used from the outside.

Copy link
Copy Markdown
Contributor Author

@Zielon Zielon Apr 1, 2019

Choose a reason for hiding this comment

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

In the current configuration is it impossible to introduce custom changes to the pick material and the pick render target. For instance, some properties for a material are not considered in the updatePickMaterial(). The same with the render target in updatePickRenderTarget().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, but why would you want to modify those? The pick material should have only the bare minimum to be able to perform picking. The fact that we are using a material and render-target at all is just an implementation detail. We could just as easily use the octree and do the picking in the CPU and the client shouldn't have to care about that.

Maybe if you provide an example of the exact thing you are trying to accomplish, then we could come up with some option/param to make it possible through the public interface of the octree.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the case of multiple viewports you have to be able to correctly set a current viewport size for the pick render target and scissors. In the case of a material there is for example a property useFilterByNormal which reduces the number of visible points. Moreover, when someone introduces custom features for shaders, you need to reflect those changes in the pick material as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So how about having an optional callback in the pick params that would be called with the material and render target after or in updatePickMaterial()?

Zielon added 2 commits April 1, 2019 19:14
…assed

Prevent doing unnecessary work when pixelPos is passed in params
Addition of comments
@Zielon Zielon force-pushed the feature/pixel_pos_from_params_pick branch from c5da889 to e83a2f9 Compare April 1, 2019 17:43
Copy link
Copy Markdown
Contributor

@hccampos hccampos left a comment

Choose a reason for hiding this comment

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

Cool, LGTM. I will merge and release a new version to npm

@hccampos hccampos merged commit 660bcf3 into pnext:master Apr 2, 2019
hccampos pushed a commit that referenced this pull request Apr 2, 2019
…` to pick params (#36)

- `pixelPosition`: If provided, the picking will use this pixel position instead of the `Ray` passed to the `pick` method.
- `onBeforePickRender`: Function which gets called after a picking material has been created and setup and before the point cloud is rendered into the picking render target. This gives applications a chance to customize the renderTarget and the material.
@hccampos
Copy link
Copy Markdown
Contributor

hccampos commented Apr 2, 2019

@Zielon I released v0.1.0. I modified the pick params slightly to have a bit better/more consistent names:

  • pixelPosition instead of pixelPos
  • onBeforePickRender instead of pickConfiguration to match the onBeforeRender of ThreeJS.

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