3D Tiles Points updates - quantization and oct-encoding #4183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I can do this unless you prefer to. |
|
Part of CesiumGS/3d-tiles#22. |
|
Should CI be failing? https://travis-ci.org/AnalyticalGraphicsInc/cesium/builds/150765222 I thought we fixed it for 3d-tiles. |
Despite our experiences elsewhere, I don't think this assumption is true today and it definitively won't be true moving forward as we do more work with interiors and local data acquisition. I also think that most of the datasets @tfili and I worked with in early point cloud prototypes needed to be converted to WGS84.
The bounding volume and quantized volume are apples and oranges. Coupling them like this is (1) likely to be confusing to folks learning the spec and (2) hurts flexibility moving forward. |
| @@ -261,7 +261,7 @@ define([ | |||
| * @memberof UniformState.prototype | |||
| * @private | |||
| */ | |||
| inverseTranposeModel : { | |||
| inverseTransposeModel : { | |||
There was a problem hiding this comment.
Good catch. Can you please open a PR into master for this and the Model.js update?
There was a problem hiding this comment.
Actually I realized this is done already in #4115 which hasn't been merged yet.
It's related to this: #4101 (comment) |
| }); | ||
| }; | ||
|
|
||
| content.state = Cesium3DTileContentState.READY; |
There was a problem hiding this comment.
Minor, but I think it would be cleaner to move these instead the if statement in update so this function is more cohesive as "create resources", not "create resources and set a bunch of state."
You actually need #4184 too. That PR should be merged into master and then all 3d-tiles branches need to be updated. |
| } | ||
|
|
||
| // Update the model matrix | ||
| var boundingSphere = this._tile.contentBoundingVolume.boundingSphere; |
There was a problem hiding this comment.
Is it easy/worth-it to have a dirty flag to avoid burning this bit of CPU? A full 4x4 matrix compare would not be worth it.
I only ask because this is per tile CPU only head, per tileset would not be an issue.
There was a problem hiding this comment.
Yeah this should be easy.
Yeah. I'm going to undo one change though.
Go ahead. It will probably be faster that way.
I agree, I'll make the change. |
| } | ||
|
|
||
| vs += ' normal = czm_normal * normal; \n' + | ||
| ' color *= max(dot(normal, czm_sunDirectionEC), 0.0); \n'; |
There was a problem hiding this comment.
I never really thought beyond "per-point normals for lighting." Are you sure that all we want to do for now is this diffuse term? Not the Cesium built-in GLSL functions? We can also revisit this soon when we do point cloud styling.
There was a problem hiding this comment.
For now this seems good enough, I will use getLambertDiffuse though.
|
Can you add an example tileset to the Sandcastle example that includes per-point color and quantized normals? It is good for illustration and for manual testing. |
|
Do you get test failures? I have 11 test failures for
|
|
Do you think the feature table should have an optional |
For what? I am not following. I would expect it to have the same properties for the quantized volume as i3dm. |
|
I mean if the points aren't quantized, do we want a property that says where the center is so that they can also be defined RTC. |
|
Ah, mirror the RTC glTF extension, https://github.com/KhronosGroup/glTF/blob/master/extensions/Vendor/CESIUM_RTC/README.md, maybe |
|
This is updated. |
|
Before merging, should we rename |
Yes, good eye. |
|
Updated. |
|
+1 from me. Retarget this PR to the |
|
Quick fix: renamed the folders and test tilesets from |
3 tasks
|
I retargetted to |
|
Code looks good. Will do final testing and merge after merging #4256. @lilleyse @lasalvavida would like to merge #4256 early tomorrow if possible. @lilleyse please finish the implementation before updating the spec. |
|
Spec updates: CesiumGS/3d-tiles#125 |
|
I'd like to merge this, but I get 30 test failures in this branch and just 1 in This branch:3d-tiles |
|
I'm not getting any errors. Was your test on mac or windows? |
|
That test is fixed now. |
|
Looks good! If there's any other spec changes due to CesiumGS/3d-tiles#125, open a new PR into |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge #4101 first. I made a few changes to
Cesium3DTileFeatureTableResourcessimilar to my comments in #4101 but otherwise all the new code is inPoints3DTileContent.If the changes in here look okay, we can update https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/master/TileFormats/Points accordingly.
The feature table can have the following properties:
POINTS_LENGTH(likeINSTANCES_LENGTHit's required)POSITIONorPOSITION_QUANTIZED(one of the two is required)NORMALorNORMAL_OCT16P(optional)RGB,RGBA,CONSTANT_RGBA(renamed fromCOLOR)QUANTIZED_VOLUME_SCALEandQUANTIZED_VOLUME_OFFSETRTC_CENTER(optional)To-do: