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

[PointMaterial] Custom attenuated points parameters #2171

Merged

Conversation

ketourneau
Copy link
Contributor

@ketourneau ketourneau commented Aug 23, 2023

Description

Allow custom attenuatedpoints parameters

Motivation and Context

We need to adjust attenuated points parameters for a better rendering in our case.

@ketourneau ketourneau force-pushed the custom-adaptive-points-parameters branch from 2a2a328 to a925aff Compare August 23, 2023 08:22
@jailln jailln requested a review from AnthonyGlt August 23, 2023 09:52
Copy link
Contributor

@AnthonyGlt AnthonyGlt left a comment

Choose a reason for hiding this comment

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

Thank you for the MR.
Could you clean your commits to only keep the last one. The "package" files shouldn't be edited

@AnthonyGlt
Copy link
Contributor

This is a nice feature, could you make these parameters (minAdaptiveSize/maxAdaptiveSize/adaptiveScale) editable by the user when he's loading a pointcloud using a C3DTilesLayer?
Like what was done with the pntsMode parameter. There is an example in examples/3dtiles_pointcloud.html.
By searching where pntsMode is called, it could be possible to implements the same process with pntsMinAdaptiveSize/pntsMaxAdaptiveSize & pntsAdaptiveScale .
(could be helpful #2042)
Don't hesitate to ask if help is needed

@AnthonyGlt
Copy link
Contributor

Maybe could be a good idea to introduce a parameter size_mode: ...., // PNTS_SIZE.ADAPTIVE || PNTS_SIZE.VALUE that would automatically set the size at 0 (when PNTS_SIZE.ADAPTIVE is set) and use minAdaptiveSize/maxAdaptiveSize/adaptiveScale defined by the user.
WDYT ?

@ketourneau ketourneau force-pushed the custom-adaptive-points-parameters branch 3 times, most recently from e702ac1 to 7d0e8a1 Compare August 25, 2023 09:13
@ketourneau ketourneau force-pushed the custom-adaptive-points-parameters branch from 7d0e8a1 to d7fdc3d Compare August 25, 2023 09:40
@ketourneau
Copy link
Contributor Author

@AnthonyGlt Thx for your code review.
I applied the change.
I add size_mode parameter, I think it's a more intuitive way to set point size behaviour.
Is everything good ?

@ketourneau ketourneau changed the title [PointMaterial] Custom adaptive points parameters [PointMaterial] Custom attenuate points parameters Aug 30, 2023
@ketourneau ketourneau force-pushed the custom-adaptive-points-parameters branch from 489f101 to 721b10e Compare August 30, 2023 15:22
@AnthonyGlt
Copy link
Contributor

Looks good to me. After tests, does it answer your needs ?

src/Layer/C3DTilesLayer.js Outdated Show resolved Hide resolved
@@ -160,6 +160,7 @@ class PointCloudLayer extends GeometryLayer {
this.material.opacity = this.opacity;
this.material.transparent = this.opacity < 1;
this.material.size = this.pointSize;
this.material.preSSE = context.camera.preSSE;
Copy link
Contributor

Choose a reason for hiding this comment

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

@Desplandis we should implement something similar in C3DTilesLayer, for now only the default preSSE value (1.0) will be used in the material when using a C3DTilesLayer, right ?

Concerning this MR, I think your implementation is enough.

@ketourneau ketourneau changed the title [PointMaterial] Custom attenuate points parameters [PointMaterial] Custom attenuated points parameters Sep 1, 2023
@AnthonyGlt AnthonyGlt self-requested a review September 1, 2023 09:33
@AnthonyGlt AnthonyGlt merged commit 6db3c5e into iTowns:master Sep 1, 2023
7 checks passed
@ketourneau ketourneau deleted the custom-adaptive-points-parameters branch September 1, 2023 11:37
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