-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add splitDirection property for PointPrimitive and Billboards #11982
Add splitDirection property for PointPrimitive and Billboards #11982
Conversation
Added the splitDirection property to PointPrimitive. Updated PointPrimitive Collection and shaders to work with the property.
Added the splitDirection property to PointPrimitive. Updated PointGraphics and PointVisualizer.
Updated Specs PointPrimitive Collection, Point Graphics and PointVisualizer to work with the new splitDirection property
Thank you for the pull request, @YunVlad! ✅ We can confirm we have a CLA on file for you. |
Awesome, thanks for the contribution @YunVlad! We'll review this shortly! |
Fixed formatting in PointGraphics, PointVisualizer, PointPrimitive, PointGraphicsSpec
Resolving conflict
Added the splitDirection property to Billboard. Updated BillboardCollection and shaders to work with the property. Updated BillboardGraphics and BillboardVisualizer.
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.
CHANGES.md
Outdated
@@ -1,5 +1,7 @@ | |||
# Change Log | |||
|
|||
- Added SplitDirection property for display PointPrimitive relative to the `Scene.splitPosition`. |
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.
Could you please
- merge in the
main
branch and resolve any conflicts - Add a link to this PR, ie
[11982](https://github.com/CesiumGS/cesium/pull/11982)
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.
It was done.
I also added an example in Sandcastle.
@@ -497,6 +500,12 @@ function createVAF(context, numberOfPointPrimitives, buffersUsage) { | |||
componentDatatype: ComponentDatatype.FLOAT, | |||
usage: buffersUsage[DISTANCE_DISPLAY_CONDITION_INDEX], | |||
}, | |||
{ | |||
index: attributeLocations.splitDirection, |
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.
To save on the number of attributes, which is limited, let's pack this single float component along with distanceDisplayConditionAndDisableDepth
as a 4th component.
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.
Thank you for the comment.
Fixed, added splitDirection as the 4th component in distanceDisplayConditionAndDisableDepth.
Add Sandcastle example for SplitDirection property for Points
The splitDirection attribute was packaged as the 4th component to distanceDisplayConditionAndDisableDepth. Updated PointPrimitiveCollection and shader.
Thanks for the updates @YunVlad! I noticed one bug during my testing: When adding a point with |
I found out why the bug occurs. Until the heightReference parameter is applied, points are rendered by PointPrimitiveCollection shaders (and splitDirection works as intended). However, after applying heightReference, another shader is clearly responsible for rendering (and, unfortunately, it does not work with splitDirection). But I can't figure out which one. Could you help with this question? I would also like to note that PointPrimitive does not have a heightReference parameter. |
When clamped to ground, points are then represented by billboards rater than Point Primitives. I believe for this to work with the Entity API, you'll also need to implement splitDirection for billboard primitives, namely in |
Fixed a bug where the splitDirection property stopped working when applying the heightReference parameter. Updated the example for Billboards.
Updated Specs BillboardCollectionSpec, BillboardGraphicsSpec and BillboardVisualizerSpec to work with the new splitDirection property. Updated PointVisualizerSpec to work with billboard example.
Added the splitDirection property for Billboards. Completely by analogy with the one proposed above for Points. The bug specified in the local example no longer occurs. |
Hi there, is there any chance this PR will be merged anytime soon? This feature looks really cool! |
Thanks @YunVlad! Apologies for the delay! This code is looking good. I'm going to go ahead and push a few small updates to make this feature ready for the next release. |
Description
Add the splitDirection property for PointPrimitive and Billboards.
Objects for which splitDirection is set as SplitDirection.NONE (by default) will be displayed to the right and left of Scene.splitPosition.
Objects for which splitDirection is set as SplitDirection.LEFT will be displayed to the left of Scene.splitPosition.
Objects for which splitDirection is set as SplitDirection.RIGHT will be displayed to the right of Scene.splitPosition.
This property is necessary for marking points of interest and subsequent analysis of changes over time.
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change