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

Added PointGeometry and PointAppearance #3203

Merged
merged 5 commits into from
Nov 14, 2015
Merged

Conversation

lilleyse
Copy link
Contributor

For #3177

Added PointGeometry and PointAppearance from 3d-tiles and added tests.

In order to update the bounding spheres correctly for points, I added a function to Primitive called updateBoundingVolumes. It is specific to PointAppearance though, so it seems a little out of place.

I noticed that a lot of other files were doing the same point size calculations, so I created a common function Camera.getPixelSize. Maybe this function belongs in a different file.

Primitive,
createScene) {
"use strict";
/*global jasmine,describe,xdescribe,it,xit,expect,beforeEach,afterEach,beforeAll,afterAll,spyOn*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer needed once you merge in master, delete this line.

@mramato
Copy link
Contributor

mramato commented Nov 13, 2015

I know 3D Tiles is the ultimate answer, but is there a reason we are keeping PointGeometry marked as private?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 13, 2015

I know 3D Tiles is the ultimate answer, but is there a reason we are keeping PointGeometry marked as private?

Yes, because we are still working on the points 3D Tiles format and we don't want to have a breaking change if we need to add a fast path. This will become public when ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 13, 2015

I created a common function Camera.getPixelSize. Maybe this function belongs in a different file.

Thanks, this is useful cleanup. It is fine where it is, but it can't be public as is since it takes a Context parameter, which is a private type. Instead, it could take drawingBufferWidth and drawingBufferHeight.

@mramato
Copy link
Contributor

mramato commented Nov 13, 2015

This will become public when ready.

That makes sense, thanks for the clarification. We might want to use this eventually for static Entity points as well.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 13, 2015

We might want to use this eventually for static Entity points as well.

👍. It will scream compared to the current point collection.

@@ -0,0 +1,139 @@
/*global defineSuite*/
defineSuite([
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this how the other geometry/appearance specs are organized? I feel like we could combine the point geometry/appearance tests into one file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah they are usually split up this way. The geometry specs are simpler just checking default values and helper functions, and the appearance spec does the rendering.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, keep it like this if you prefer.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 13, 2015

In order to update the bounding spheres correctly for points, I added a function to Primitive called updateBoundingVolumes. It is specific to PointAppearance though, so it seems a little out of place.

I'm fine with this approach for now. Once we have a second use case, e.g., billboards or something else sized in pixels, we can add an explicit property to all appearances that needs to be implemented (the default would be to "increase" the bounding volume by zero pixels) so Primitive is not looking for the uniform name.

You're also welcome to do this now if you want since I'm pretty confident it would not require a breaking change later.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 13, 2015

Just those comments. This looks good.

@lilleyse lilleyse force-pushed the point-geometry-appearance branch from c9d6cf8 to 12451a3 Compare November 14, 2015 16:32
@lilleyse
Copy link
Contributor Author

Updated. I held off on adding a pixelSize property to Appearance, but I did add it to PointAppearance.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 14, 2015

Looks great, thanks @lilleyse.

Can you merge master into 3d-tiles? In general, can you take ownership of the 3d-tiles branch and make sure that we merge in master frequently?

pjcozzi added a commit that referenced this pull request Nov 14, 2015
…pearance

Added PointGeometry and PointAppearance
@pjcozzi pjcozzi merged commit 015b0b3 into master Nov 14, 2015
@pjcozzi pjcozzi deleted the point-geometry-appearance branch November 14, 2015 16:41
@lilleyse
Copy link
Contributor Author

Sure I can do that.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 14, 2015

Awesome, thanks!

@pjcozzi pjcozzi mentioned this pull request Nov 24, 2015
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.

3 participants