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

Crash while resizing a brush #1708

Closed
christopher-rieder opened this issue Feb 3, 2017 · 4 comments
Closed

Crash while resizing a brush #1708

christopher-rieder opened this issue Feb 3, 2017 · 4 comments
Assignees
Labels
Prio:1 Highest priority: Crash or crippling bugs, features that enable new ways of working Type:Bug Errors and problems

Comments

@christopher-rieder
Copy link

crash.zip
TrenchBroom Version: 2.0.0
TrenchBroom Build: v2.0.0-RC3 Release
Reason: TrenchBroomUnhandledExceptionFilter

i dragged the mouse to the edge of the screen

https://drive.google.com/open?id=0B72Jjyg-RfNFN3F4N2lRS2gxRm8

@ericwa
Copy link
Collaborator

ericwa commented Feb 3, 2017

I can reproduce this by clipping the starting cube like this:
screen shot 2017-02-02 at 7 55 58 pm
then zooming out and resizing the face on the right side.
In ResizeBrushesTool::PickProximateFace::visitEdge(), I get null here:
Model::BrushFace* left = edge->firstFace()->payload();

@christopher-rieder
Copy link
Author

also, this:
https://drive.google.com/open?id=0B72Jjyg-RfNFV2pBcWU1ZnFmb0U

i think that is what happened
when resizing a brush outside of the boundaries of the world (or whatever is the term)

@kduske kduske added this to the TrenchBroom 2.0.0 milestone Feb 3, 2017
@kduske kduske added Platform:All Prio:1 Highest priority: Crash or crippling bugs, features that enable new ways of working Type:Bug Errors and problems labels Feb 3, 2017
@ericwa
Copy link
Collaborator

ericwa commented Feb 26, 2017

I'm looking into this

@ericwa ericwa self-assigned this Feb 26, 2017
@ericwa
Copy link
Collaborator

ericwa commented Feb 26, 2017

Well, I wrote a test that doesn't trigger the problem :-(

        TEST_F(MapDocumentTest, resizePastWorldBounds) {
            Model::BrushBuilder builder(document->world(), document->worldBounds());
            Model::Brush *brush1 = builder.createBrush(Vec3::List{Vec3(64, -64, 16), Vec3(64, 64, 16), Vec3(64, -64, -16), Vec3(64, 64, -16), Vec3(48, 64, 16), Vec3(48, 64, -16)}, "texture");
            document->addNode(brush1, document->currentParent());
            document->select(brush1);

            Model::BrushFace *rightFace = brush1->findFace(Vec3(1,0,0));
            ASSERT_NE(nullptr, rightFace);
            
            ASSERT_FALSE(document->resizeBrushes(Polygon3::List{rightFace->polygon()}, Vec3(7000,0,0)));
        }

However, I think I see what's happening: we hit throw GeometryException("Brush is not fully specified"); in Brush::rebuildGeometry. the rebuildGeometry call is at the end of Brush::moveBoundary.

  • First problem, we probably never should have reached this because the move should have been rejected by canMoveBoundary.
  • Second problem, which is why it crashes: As the exception is bubbling up the stack, the NotifyBeforeAndAfter's run in MapDocumentCommandFacade::performResizeBrushes. This causes ToolBoxConnector::updatePickResult to run, but the brush is in an invalid state at this point (it's missing a face), so we get the crash.

I can fix the first problem and write a test that the move is rejected by canMoveBoundary, this should fix this particular issue.

ericwa added a commit that referenced this issue Feb 26, 2017
kduske pushed a commit that referenced this issue Mar 21, 2017
* #1708: Add failing test

* #1708: Fix case when canMoveBoundary incorrectly returns true
@ericwa ericwa closed this as completed Mar 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Prio:1 Highest priority: Crash or crippling bugs, features that enable new ways of working Type:Bug Errors and problems
Projects
None yet
Development

No branches or pull requests

3 participants