-
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 individual model projection in 2D / CV for `ModelExperimental #10419
Conversation
Thanks for the pull request @j9liu!
Reviewers, don't forget to make sure that:
|
@j9liu looking at the sandcastles you posted, I had some questions: 2D / CV Sandcastle:
|
@j9liu looking at the sandcastles you posted, I had some questions: 2D / CV Sandcastle:
|
|
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'm liking the new ModelExperimentalDrawCommand
architecture, it feels a lot more straightforward than what Model
was doing, and it simplifies quite a bit of the rest of the code.
I had some comments, mostly small suggestions or clarification questions
Source/Scene/Model.js
Outdated
if ( | ||
frameState.mode === SceneMode.SCENE2D && | ||
(boundingVolume.center.y + boundingVolume.radius > idl2D || | ||
boundingVolume.center.y - boundingVolume.radius < idl2D) | ||
left < idl2D && | ||
right > idl2D |
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.
Also, is there a reason why this code isn't checking a similar conditions at y=0
?
e.g. if left < 0 && right > 0
wouldn't that also be straddling the IDL?
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'm not sure what you mean, does the IDL wrap around to y = 0
?
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.
Okay, after some debugging, I found that it was supposed to check if left < -idl2D && right > -idl2D
as well. so the general idea was right
@j9liu this is looking pretty close, though there's a small merge conflict in the changelog. I also noticed that in 2D mode, the propellers on the drone disappear on one side: I wondered if the individual primitives in the model are being translated to different copies of the IDL since each primitive bounding sphere is different. I tried modifying the Sandcastle to put the model close to the edge but not quite. I notice that at |
@ptrgags updated! |
@j9liu looks good and I confirmed that you fixed the missing primitives. Thanks! |
This PR closes #9934. It allows models loaded in with
ModelExperimental
to render in 2D / CV, without needingprojectTo2D
enabled. This is ideal for individual models that aren't part of a 3D Tiles tileset.This required refactoring the draw command system of
ModelExperimental
; now there is aModelExperimentalDrawCommand
class that handles a primitive's draw command, and any other commands derived from it.I also found that the check for adding commands in 2D for
Model
was incorrect; it would always evaluate to true, so it was always adding the 2D commands even if the model was nowhere near the IDL.Sandcastles for testing:
@ptrgags can you review?