-
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 2D / CV support to tilesets using ModelExperimental #10384
Conversation
Thanks for the pull request @j9liu!
Reviewers, don't forget to make sure that:
|
One idea - maybe the instanced transforms get projected but the geometry doesn't? That way it's like a hybrid of the 3D Tiles approach and the Model approach. |
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.
@j9liu I did a light first pass review, pointed out some questions I have before I review it further.
I think one big question is when we consider the shader as a whole, what parts use the 3D position and what parts use the 2D position? If they all use one or the other, could a_positionMC
store either and the only difference is the matrices?
const buffer = Buffer.createVertexBuffer({ | ||
context: frameState.context, | ||
typedArray: projectedPositions, | ||
usage: BufferUsage.STATIC_DRAW, | ||
}); | ||
buffer.vertexArrayDestroyable = false; | ||
|
||
// Store the reference point for repeated future use. | ||
attribute.buffer2D = buffer; | ||
attribute.referencePoint2D = referencePoint; |
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.
Hm, the typical pattern we've been using is to add generated resources to model._resources
, and whenever the pipeline is rebuilt, those resources are .destroy()-ed
to start over fresh. Makes it easier to manage the memory, but perhaps is more wasteful if things can be cached. We might want to discuss this more
407f657
to
5f6128a
Compare
@ptrgags I think this is ready for another look! |
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.
@j9liu did a pass through the code. Overall the architecture looks a lot cleaner now that the 2D logic is in its own pipeline stage!
I had some ideas about better managing buffers/typed arrays in the loaders, and a few other things here and there.
I still need to try some things out in Sandcastle, I'll do that next.
@j9liu I tried the Sandcastle links you gave me, noticed a couple things: In the model one the drone propellers and ground vehicle's wheels aren't rotated correctly. Is this expected? (I know some details will be in a future PR) Also I notice the animation doesn't seem to happen in 2D/CV, is that expected behavior? |
@ptrgags found the error, it was because nodes that used the same mesh were modifying the same typed array, instead of calling Here's a sandcastle for testing two projected instances of the same model. |
@ptrgags this is ready for another look, whenever you can! |
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.
@j9liu looks pretty close, though I noticed a bug when
Also I tried the S2 Base Globe in 2D/CV mode, it's mostly working (aside from the IDL wrapping, I know that's a future PR.)
The one thing I'm uncertain of is whether the S2 bounding volumes are handled correctly in 2D. Not sure how bounding volume intersections are handled in projected coords. Maybe take a quick look and see if this is correct behavior or something we need to open an issue for?
initialPosition | ||
); | ||
|
||
const projectedPosition = SceneTransforms.computeActualWgs84Position( |
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 was trying the San Francisco Ferry Building Sandcastle and this line seems to hit a divide by zero error when I zoom in to the tileset.
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 noticed a similar problem in this sandcastle for OSM buildings.
I'm wondering if maybe there's a byteStride we're not accounting for here?
This morning @j9liu and I looked at the code for bounding volumes in 2D - it looks like Cesium3DTile always uses a bounding sphere around the projected positions for 2D, so that probably explains why it doesn't react the way I was expecting. I don't think there's any action needed. |
Turns out that the sandcastles that were crashing had data with @ptrgags ready for review, hope this can be merged soon! |
@j9liu I can confirm that you fixed the I notice that for OSM buildings, NYC is working fine, but Tokyo or Sydney don't show up. Though I think that's beyond the scope of this PR, probably has more to do with tile bounding volumes than I'm merging this now, I know everything else will be handled in future PRs |
This PR adds 2D / CV projection to
ModelExperimental
with theprojectTo2D
option. This option is always set to true by tilesets, and it projects the positions more accurately than the oldModel
class. Here's a preview of the results:Model
ModelExperimental
TODO:
ModelExperimental
constructor to enable accurate projections. This will default to false for models loaded in withfromGltf
, but should always be true for models loaded in from a tileset.enableProjection2D
?projectTo2D
?Add old-- will be done in a separate PRModel
projection code toModelExperimental
for models that aren't associated with a tileset. This allows models to show up in 2D / CV mode without breaking from options likeminimumPixelSize
ModelExperimental
(only happens in 2D/CV)Fix instancing errors -- current method doesn't support instancing-- save for another PRThis is related to #9934, but the issue shouldn't be closed until
ModelExperimental
gets the oldModel
projection code for individual models (will be a separate PR).