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

Pasting entities aren't always snapped to grid #2754

Closed
bstordeur opened this issue May 8, 2019 · 4 comments · Fixed by #2761
Closed

Pasting entities aren't always snapped to grid #2754

bstordeur opened this issue May 8, 2019 · 4 comments · Fixed by #2761
Assignees
Labels
Prio:2 Medium priority: Non crash bugs that impede the user, features that add new functionality. Type:Bug Errors and problems
Milestone

Comments

@bstordeur
Copy link

bstordeur commented May 8, 2019

Not sure when this started, but for a few versions now pasting some entities doesn't align them to the grid as it did before.
Here's an example, I copy and paste an monster_enforcer in latest TB, notice how its bounding box is not aligned properly to grid when I paste :
pastingsnap

Now if open some older version of TB (2019.4 here, so not that old), if I do the same thing, it snaps as it always has :
pastingworks

System Information

TrenchBroom 2019.5 Build v2019.5-RC5-1-gf002ad8 Release on Windows 7.

Expected Behavior

Like it was before, snap the bounding box of the entity to the current grid setting.

Steps to Reproduce

Create a monster_enforcer (or most other entities), copy it, and paste it, notice how it's no longer aligned to the grid.

@kduske
Copy link
Collaborator

kduske commented May 8, 2019

Probably using the wrong bounds for alignment. Are the gifs switched?

@bstordeur
Copy link
Author

The gifs were switched when I first posted the issue, changed it right away though.
It looks like it only happens with ents that have a model, almost like it's using the models bounding box (which you can see when you hover over them).

@ericwa ericwa added Prio:2 Medium priority: Non crash bugs that impede the user, features that add new functionality. Type:Bug Errors and problems labels May 10, 2019
@ericwa ericwa added this to the 2019.6 milestone May 10, 2019
@ericwa ericwa self-assigned this May 11, 2019
@ericwa
Copy link
Collaborator

ericwa commented May 11, 2019

The bounds displayed when you mouseover a monster changed too, now it includes the model.
We should revert that to the old behaviour.

@ericwa
Copy link
Collaborator

ericwa commented May 11, 2019

@kduske There are different ways to approach this but somehow we need to separate "culling bounds" from the "semantic bounds" in Node's API.

What I'm thinking is Node::bounds() should be the semantically important bounds - so for point entities, this is the Entity::definitionBounds(), and we should add a new Node::cullingBounds() that is the maximum bounds including arms poking out, etc., used for rejecting hits when doing hit detection, frustum culling.

For this particular issue, the paste code is using MapDocument::selectionBounds() to place the pasted objects, so that needs to use the semantic bounds.

Capture

After using it a bit I'm not sure if there's really a point to drawing the outer box. Or if we do keep it, the text overlay should be describing the inner bounds, anyway.

ericwa added a commit that referenced this issue May 12, 2019
Add a new Node::cullingBounds() that includes the model bounds.

Fixes #2754
ericwa added a commit that referenced this issue May 16, 2019
)

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

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

Fixes #2754

* 2754: fix initializer order

* 2754: fix a few bounds() calls that should be cullingBounds()

* 2754: add docs

* 2754: ComputeNodeBoundsVisitor: use bbox::builder

* 2754: don't add Group to node tree

* 2754: cullingBounds->physicalBounds refactor

* 2754: Node::bounds->logicalBounds refactor

* 2754: more bounds refactoring

* 2754: rename *BoundsDidChange functions to *PhysicalBoundsDidChange

* 2754: fix comment

* 2754: minor naming adjustments / comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Prio:2 Medium priority: Non crash bugs that impede the user, features that add new functionality. Type:Bug Errors and problems
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants