Crash when snapping verts to grid in vertex mode

[PritchardGSD]

lavabloom_quoth-crash-12.zip
lavabloom_quoth-crash-13.zip
Not much to say about this one. Zip file includes a screenshot of TB when I crashed, the vert with the error is pictured.

To reproduce:
Take this brush:

Cut it like this:

Extend it with shift-drag like this:

Vertex manipulate like this:

Then switch grid size to 1 and "Snap vertices to Grid"

On my system at least this produces a crash.

[kduske]

I will not be able to reproduce this. Can you please attach the brush to which you apply the snap operation? That way, all I have to do is select it and perform the snap.

[PritchardGSD]

crash.zip
This is the best I can do. The map I started to hold /just/ the brushes that I think are affected wouldn't always crash, so you may have trouble. But it did crash once after trying to undo the snap operation, and I included the files that generated.

[PritchardGSD]

One issue is that it's not 100% consistent, so sometimes it seems to snap correctly, and it always seems to snap correctly when loading the map from the report so getting the brushes right as they're about to crash seems to be a bit of guesswork.

[kduske]

Yeah, I need a consistent and reliable reproduction, otherwise I'm just guessing. Maybe @ericwa has an idea?

[ericwa]

It looks like the crash is just happening when snapping in vertex mode, which I can reproduce.

1. New map (standard format)
2. Select the cube
3. Press v to enter vertex mode
4. Edit -> Snap vertices to integer, this will crash in VertexHandleManager::removeBrushes

[kduske]

Excellent, thank you @ericwa. I'll look into it.

[kduske]

@ericwa unless you want to ;-)

[ericwa]

@kduske Looks like VertexTool::isVertexCommand is returning true for SnapBrushVerticesCommand, but SnapBrushVerticesCommand is not actually a VertexCommand subclass.
So the static_cast here: https://github.com/kduske/TrenchBroom/blob/release/v2.0.0/common/src/View/VertexTool.cpp#L552 gives a bogus pointer and then things blow up.

Maybe replace:

if (isVertexCommand(command)) {
    VertexCommand* vertexCommand = static_cast<VertexCommand*>(command.get());
    ...

with:

if (VertexCommand* vertexCommand = dynamic_cast<VertexCommand*>(command.get())) {
    ...

[kduske]

No, I don't want to rely on dynamic_cast. Maybe it should actually be a VertexCommand subclass since it does actually change the vertices. No idea what I was thinking there, to be honest.

[ericwa]

Are you avoiding dynamic_cast altogether in TB? The VertexCommand* vertexCommand = static_cast<VertexCommand*>(command.get()); line is really dangerous with static_cast because you get memory clobbering if there happens to be a bug in isVertexCommand, as in this case.

[kduske]

Yes, I am. In fact, I'm trying to avoid downcasting altogether. In this instance it's dangerous, yes. But downcasts are very often a design smell, and dynamic casts (and instanceof etc. in Java) just hide the smell a little better so that it's harder to detect.

[kduske]

Fixed in 2380404. Thanks @ericwa for finding the problem.