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

2754: Revert Entity::bounds() to returning the definition bounds. #2761

Merged
merged 13 commits into from
May 16, 2019

Conversation

ericwa
Copy link
Collaborator

@ericwa ericwa commented May 12, 2019

Add a new Node::cullingBounds() that includes the model bounds.

I need to add some docs, but this is what I had in mind in the comment here: #2754 (comment)

cullingBounds() is always equal or larger than bounds(). Currently, the only place where it differs from bounds() is it includes entity models that extend beyond the entity definition bounds, like monster_boss. The only place it's used is for AABB tree insertions.

Entity::definitionBounds could be dropped and replaced with bounds() now.

I should double check the codebase for any uses of bounds() that should be changed to cullingBounds(), but this seems to work.

Fixes #2754

Add a new Node::cullingBounds() that includes the model bounds.

Fixes #2754
@ericwa ericwa requested a review from kduske May 12, 2019 08:19
@ericwa
Copy link
Collaborator Author

ericwa commented May 12, 2019

That should be everything. Was wondering if we should rename Node::bounds() to Node::gameplayBounds() but maybe bounds is enough.

This stuff: Object::contains, Object::intersects, BoundsIntersectsNodeVisitor, CollectTouchingNodesVisitor, is using Node::bounds() so it will not take into account an arm sticking out of the entity definition bounds. This is what we want for the "Select Touching" command, at least.

Copy link
Collaborator

@kduske kduske left a comment

Choose a reason for hiding this comment

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

I approve of the changes, but object to the naming.

common/src/Model/Brush.cpp Outdated Show resolved Hide resolved
common/src/Model/ComputeNodeBoundsVisitor.cpp Show resolved Hide resolved
common/src/Model/Node.h Outdated Show resolved Hide resolved
common/src/Model/Node.h Show resolved Hide resolved
@ericwa ericwa requested a review from kduske May 15, 2019 06:49
Copy link
Collaborator

@kduske kduske left a comment

Choose a reason for hiding this comment

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

Good to have this clarified finally.

@ericwa ericwa merged commit 649b9b6 into master May 16, 2019
@ericwa ericwa deleted the feature/2754 branch May 16, 2019 01:40
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.

Pasting entities aren't always snapped to grid
2 participants