Why does this cause sketchup to crash?

I created a model observer to detect when a component is placed. I want to explode that component after it is placed. I tried doing it this way, but it causes Sketchup to crash.

    class MyModelObserver < Sketchup::ModelObserver
           def onPlaceComponent(instance)
            puts "onPlaceComponent: " + instance.to_s

Well for one thing if you explode an instance, it no longer exists. Your calling instance.to_s after you’ve made it go POOF!

Secondly… the SketchUp engine has a reference to the instance, and your observer may only be one of several “standing in line” waiting to receive notice that the instance has been placed.
Other observer’s onPlaceComponent callbacks may not atcually test the instance reference to see if it’s valid?, which might cause the crash. (Notice how you didn’t?)

Thirdly, the API told you NOT to do this:

Performing any edit operations of your own (such as modifying the model) inside the observer callback should be avoided, as it could cause crashes or model corruption.

(It is from the Introduction of the ModelObsever class documentation page.)

Fouthly… model edits should be wrapped in a undo operation (see Model#start_operation) and operations should be wrapped in a beginrescueend block (with a call to abort_operation in the rescue clause.)

There is a fundamental problem with how observers were originally implemented - they hooked directly to the internal notifications. This means they fire in the middle of native operations. So if you modify the model during observer events you might erase entities might the native operation have completed.

For this reason there is that warning in the API docs which Dan pointed out.

We haven’t had many best practices examples for the Ruby API and this is something we want to improve.

For observers we do however have a GitHub code example: GitHub - SketchUp/sketchup-safe-observer-events: Wrapper for executing model changes from observer events safely

This demonstrate how you can defer model changes until the operation which triggered it is done. (Though note it have to resort to an ugly timer hack in order to defer because ModelObserver.onTransactionStart/Commit will trigger for intermediate Ruby operations. But currently this is the safest way to perform model changes.

Also - as Dan mentions, when you do make model changes in reaction to observers you need to make sure you handle the undo stack correctly. If you explode an instance when it’s added that will create a new undo step - meaning the user would have to perform two undo actions, which is bad for usability.

Model changes based on observer callbacks should make their operations transparent to the operation they react to. This means setting the fourth argument of model.start_operation to true. (Don’t use the third argument - we have plans to deprecate this because it’s just too unpredictable and nearly every use of it cause undesired side-effects.)

The GitHub example also demonstrate how to correctly start a transparent operation.

Note that if you call model.abort_operation during a transparent operation you will not only abort the one you started, but also the operation to chained to. So be cautious about this and consider manually cleaning up and committing instead.

1 Like

I’ll agree if it’s your own code that caused the first op, and your attaching a second op to the previous.

But blindly attaching operations to someone else’s operation is asking for trouble. IMHO.

Which is worse,… corrupting the model or aborting 2 operations ?

At the bottom of the page, in the Gremlin example, the following comment makes no sense:
“# This is raise error before the face is gone.”

If it’s possible to manually revert your changes in the chained operation I’d recommend that. If it’s something simple such as adding a group, instance or anything else which doesn’t merge with other geometry.
If aborting it would probably be good with some clear message back to the user so that the user know what function and extension caused the error.

You’re right, that make no sense at all… need to clean that one up.