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

Camera-Terrain Collision #2470

Merged
merged 11 commits into from
Feb 17, 2015
Merged

Camera-Terrain Collision #2470

merged 11 commits into from
Feb 17, 2015

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Feb 4, 2015

Add camera-terrain collision detection/response when the camera reference frame is set. This change assumes that when the camera reference is not earth fixed, the camera is always looking at the center of the frame.

CC #1060, #1936

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 5, 2015

Is it possible to add a test for this?

Also, is there a flag to turn on/off collision detection? If so, this should also consider it. If not, I suspect we'll add it at some point, but no need for it now.

@mramato
Copy link
Contributor

mramato commented Feb 5, 2015

I originally din't think we would need one (and told @bagnell as much) but the more I think about it, the more having an option to disable is probably a good idea. That being said, I'm also okay with holding off until someone asks for it. It's up to you guys.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 5, 2015

@mramato submit an issue.

@bagnell
Copy link
Contributor Author

bagnell commented Feb 5, 2015

Is it possible to add a test for this?

We could if we want the tests to wait for terrain to load. Is there any terrain providers in the test code that only loads one hi-res tile?

@bagnell
Copy link
Contributor Author

bagnell commented Feb 5, 2015

@pjcozzi @mramato This is ready for another review. I added tests for collision detection when the camera transform is the identity too.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 5, 2015

@mramato can you test and merge?

* @type {Boolean}
* @default true
*/
this.avoidCollisionInReferenceFrame = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this flag to be this specific? Why not just have a boolean that enables/disable collision detection completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because people might not want us to reorient the camera after we detect a collision. For example, they may set the camera reference frame and not look at the center, then, when we detect a collision, it will look towards the center.

I though about adding another flag that enabled/disabled collision detection altogether, but would anyone really want to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

but would anyone really want to do that?

I suspect yes, at least for subsurface.

@bagnell
Copy link
Contributor Author

bagnell commented Feb 7, 2015

@mramato This is ready for another review.

@mramato
Copy link
Contributor

mramato commented Feb 10, 2015

I would get rid of avoidCollisionInReferenceFrame because enableCollisionDetection can just be set to turned on and off as needed basis. I would also mention enableCollisionDetection in CHANGES.

@mramato
Copy link
Contributor

mramato commented Feb 10, 2015

Dan, I merged in master again so I could make a build for our other project.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 16, 2015

What's the latest here?

@bagnell
Copy link
Contributor Author

bagnell commented Feb 17, 2015

@mramato This is ready for another review.

mramato added a commit that referenced this pull request Feb 17, 2015
@mramato mramato merged commit a4425f3 into master Feb 17, 2015
@mramato mramato deleted the collision branch February 17, 2015 23:25
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