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

2195: Vertices becoming unstuck when dragging #2198

Merged
merged 4 commits into from
Aug 5, 2018
Merged

Conversation

kduske
Copy link
Collaborator

@kduske kduske commented Jul 27, 2018

Closes #2195

The problem was already caught by an assertion in debug builds -- the problem was that the vertex handle manager still used a comparator based on lexicographic compare to map handle positions. Therefore, the positions submitted to a brush would not be 100% exact and would cause a failure.

The fix is to use a different type of comparator which snaps the vertices to a grid of 0.001 before the lexicographic compare, and using the same kind of set in the brush vertex transformations.

@kduske kduske requested a review from ericwa July 27, 2018 20:06
@ghost ghost assigned kduske Jul 27, 2018
@ghost ghost added the Status:In Progress label Jul 27, 2018
@kduske kduske added this to the 2.0.6 milestone Jul 27, 2018
Copy link
Collaborator

@ericwa ericwa left a comment

Choose a reason for hiding this comment

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

I tested a bit with touching off grid brushes and it seems to work.

My only concern here is, it's possible to have nearby vertices snap to different grid locations (e.g. if the grid size is 1.0, 0.4999 snaps to 0.0 and 0.5 snaps to 1.0).

I don't know how likely this is to happen in practice. The easiest way to avoid it is just abandon the idea of sets of vertices, and test every element in a std::vector with an epsilon to see if there is a match. Not sure if it's worth it..

common/src/Vec.h Outdated

Vec<T,S>& snap(const T precision) const {
for (size_t i = 0; i < S; ++i) {
v[i] = Math::snap(v[i], precision);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see how this compiles, shouldn't modifying v fail since this snap function is const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, interesting! No compiler caught this, it seems. Good that we will get rid of these methods soon.

@kduske
Copy link
Collaborator Author

kduske commented Aug 5, 2018

Regarding your comment: The snapping here is not dependent on the grid size. The vertices are always snapped onto a grid of size 0.001. This is really only for grouping vertices together which are very close to each other.

@kduske kduske merged commit 84ed620 into release/v2.0.6 Aug 5, 2018
@ghost ghost removed the Status:In Progress label Aug 5, 2018
@kduske kduske deleted the feature/2195 branch August 5, 2018 17:49
@ericwa
Copy link
Collaborator

ericwa commented Aug 5, 2018

The same thing will happen on grid 0.001: at the half-way lines between grid lines, i.e. every 0.0005 that's not a multiple of 0.001, if the vertices are on different sides of that line, it doesn't matter how close they are, they'll get snapped to different bins.

e.g. (1.00049, 0, 0) and (1.00051, 0, 0) are only 0.00002 apart (2% of the 0.001 grid) but they get snapped to different positions.

I hit this same thing in qbsp; what I did is, when inserting vertices into the set, for each axis, insert both the floor() and ceil() rounded version, instead of just round().
https://github.com/ericwa/ericw-tools/blob/master/qbsp/surfaces.cc#L209

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.

None yet

2 participants