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

Add splitDirection property for PointPrimitive and Billboards #11982

Merged
merged 29 commits into from
Jul 26, 2024

Conversation

YunVlad
Copy link
Contributor

@YunVlad YunVlad commented May 14, 2024

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

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code
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
Copy link

Thank you for the pull request, @YunVlad!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented May 14, 2024

Awesome, thanks for the contribution @YunVlad! We'll review this shortly!

YunVlad and others added 4 commits May 14, 2024 16:22
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.
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Awesome @YunVlad!

It would be great to include sample code for this in a Sandcastle example. Would you be open to either adding a new example, or extending an existing example such as this one to showcase this feature?

CHANGES.md Outdated
@@ -1,5 +1,7 @@
# Change Log

- Added SplitDirection property for display PointPrimitive relative to the `Scene.splitPosition`.
Copy link
Contributor

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)
Copy link
Contributor Author

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,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

YunVlad and others added 4 commits May 30, 2024 09:44
Add Sandcastle example for SplitDirection property for Points
The splitDirection attribute was packaged as the 4th component to distanceDisplayConditionAndDisableDepth.
Updated PointPrimitiveCollection and shader.
@ggetz
Copy link
Contributor

ggetz commented May 30, 2024

Thanks for the updates @YunVlad!

I noticed one bug during my testing:

When adding a point with viewer.entities.add, setting heightReference to HeightReference.CLAMP_TO_GROUND causes the split property to stop working. Here's a local Sandcastle example demonstrating the issue.

@YunVlad
Copy link
Contributor Author

YunVlad commented May 31, 2024

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.

@ggetz
Copy link
Contributor

ggetz commented Jun 3, 2024

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?

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 BillboardCollectionFS.glsl.

YunVlad and others added 6 commits June 3, 2024 23:20
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.
@YunVlad
Copy link
Contributor Author

YunVlad commented Jun 4, 2024

Added the splitDirection property for Billboards. Completely by analogy with the one proposed above for Points.
Updated shaders, tests, and also updated the example for Sandcastle.

The bug specified in the local example no longer occurs.

@ilyaly
Copy link
Contributor

ilyaly commented Jul 25, 2024

Hi there, is there any chance this PR will be merged anytime soon? This feature looks really cool!

@YunVlad YunVlad changed the title Add splitDirection property for PointPrimitive Jul 25, 2024
@ggetz
Copy link
Contributor

ggetz commented Jul 26, 2024

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.

@ggetz ggetz merged commit f04e222 into CesiumGS:main Jul 26, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants