-
Notifications
You must be signed in to change notification settings - Fork 3.7k
support 2.5D/2D projection by default when the option projectTo2D is not set #12825
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for the pull request, @mickae1! ✅ We can confirm we have a CLA on file for you. |
@lukemckinstry @ggetz do you think this pull request can be accepted for the next cesium release ? My teams need it. Thank you, |
Hi @mickae1! Thanks for opening a PR and contributing this feature. I have a couple questions to kick off my review:
But beyond the actual implementation, I'm curious what it is you're trying to achieve with this new feature - maybe we already support what you want to do some other way! (Again, why it's often helpful to start with or link a forum post). I'll assign myself as a reviewer so I can get notified when you reply! |
@@ -166,6 +166,7 @@ import ModelImagery from "./ModelImagery.js"; | |||
* @privateParam {boolean} [options.showCreditsOnScreen=false] Whether to display the credits of this model on screen. | |||
* @privateParam {SplitDirection} [options.splitDirection=SplitDirection.NONE] The {@link SplitDirection} split to apply to this model. | |||
* @privateParam {boolean} [options.projectTo2D=false] Whether to accurately project the model's positions in 2D. If this is true, the model will be projected accurately to 2D, but it will use more memory to do so. If this is false, the model will use less memory and will still render in 2D / CV mode, but its positions may be inaccurate. This disables minimumPixelSize and prevents future modification to the model matrix. This also cannot be set after the model has loaded. | |||
* @privateParam {boolean} [options.customGeometryStage=false] Whether to override the default geometry stage, to let the user define it's own geometry stage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit- it's
-> its
.
But, per my overall comment, I think it might make more sense to do something like pass in the geometry stage function snippet itself rather than a flag.
Oh, also - if you haven't already, can you add yourself to |
you can find here the issue : |
for my model loaded from glb, i want to do the projection change in vertex shader when I'm in 2D/2.5D . ( i don't want any cpu computing the 2D projection ) the code works, the projection works. you can find here the sandcastle please tell me what I need to do to validate this pull request |
I've made modifications, to start a new approach, more generic to every one, please give advice . the updated sandcastle |
…not set add the options.customGeometryStageVS Whether to override the default VS geometry stage, to let the user define its own VS geometry stage.
return vec3(altitude, plate_x, plate_y); | ||
} | ||
|
||
vec3 cartesianToCartographic(vec3 cartesian) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still processing the math here, but wondering if something here might be helpful (or if we can even just use these functions directly). It looks conceptually similar.
If we can use the above, I wonder if isSphere
is actually a fast path, given the expense of trig functions on the GPU. Might be worth doing some performance measurements there to see. Maybe it's faster to just treat everything as an ellipsoid and use that non-trig iterative method.
|
||
#if defined(USE_2D_POSITIONS) || defined(USE_2D_INSTANCING) | ||
vec3 position2D = attributes.position2D; | ||
vec3 positionEC = (u_modelView2D * vec4(position2D, 1.0)).xyz; | ||
computedPosition = czm_projection * vec4(positionEC, 1.0); | ||
#else | ||
computedPosition = czm_projection * vec4(v_positionEC, 1.0); | ||
if(czm_morphTime != 1.){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit- given that, the vast majority of the time, we operate in 3D mode, it would be slightly faster to invert this check so that it short-circuits more often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Looking back at this, I see that this is within the USE_2D_POSITIONS
macro, so is it already always 2D mode? If we remove the CPU-side projection implementation, per my overall-review comment, then we would remove this macro too, I think, and we'd need the czm_morphTime
check you added.)
vec3 cartographic = cartesianToCartographic(positionMC); | ||
vec3 position = projectToGeographic(cartographic); | ||
|
||
vec3 positionEC = (czm_view * vec4(position, 1.0)).xyz; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to write out to v_positionEC
in this branch of the if-statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't know, If it is used in other pipeline, or used in fragment shader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix v_
typically means it's a vertex output attribute - so it would get used in the fragment shader. In this case, you can see that here.
|
||
#if defined(USE_2D_POSITIONS) || defined(USE_2D_INSTANCING) | ||
vec3 position2D = attributes.position2D; | ||
vec3 positionEC = (u_modelView2D * vec4(position2D, 1.0)).xyz; | ||
computedPosition = czm_projection * vec4(positionEC, 1.0); | ||
#else | ||
computedPosition = czm_projection * vec4(v_positionEC, 1.0); | ||
if(czm_morphTime != 1.){ | ||
vec3 cartographic = cartesianToCartographic(positionMC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This probably reveals some misunderstanding I have but) - why does this operate on the model space position instead of world or eye space?
Overall comment - I have yet to test the changes, will do soon. Just did an overview for the moment. I think this could be a really nice contribution - if it works as promised without any regressions, we should go a step further and remove the CPU implementation of the projection. This would be the default, and we could skip the added options to the geometry pipeline stage to override the implementation. |
@mzschwartz5 what i'm doing in the vertex shader, is exactly what is done in the cpu :
with :
I found out why, it need to be vec3(z,x,y ) to be working : ( and not vec3(x,y,z) ) : because it's what you are doing in other shader :
but not here, why ?
|
to test :
test.zip
you can find here the sandcastle