FC0.17 recompute strange behaviour (regression)

[wmayer]

So, the meaning of object.Touched flag has changed.

Old meaning: object.Touched = the object has been changed since last recompute. That does not mean the object itself must be recomputed, but it does mean that any object depending on this object must be recomputed.

New meaning: object.Touched = object must be recomputed.

Since changing any property except Label makes an object touched, it will be recomputed. mustExecute almost lost its meaning.

So an alternative solution may be to not set object.Touched upon every change to any property.

That's exactly the problem. Related to this is that the implementation of Document::recompute() before the above commit has already changed where the "touched" flag is used to mark dependent objects for recompute but actually weren't recomputed because only mustExecute was checked. So the git commit 283ab961b was also needed to fix this inconsistency.

To restore the original meaning of the "touched" flag there is now the new flag "Enforce" (with the method enforceRecompute) which is explicitly used to mark on object for recompute without touching one of its properties.

[realthunder]

That's exactly the problem. Related to this is that the implementation of Document::recompute() before the above commit has already changed where the "touched" flag is used to mark dependent objects for recompute but actually weren't recomputed because only mustExecute was checked. So the git commit 283ab961b was also needed to fix this inconsistency.

To restore the original meaning of the "touched" flag there is now the new flag "Enforce" (with the method enforceRecompute) which is explicitly used to mark on object for recompute without touching one of its properties.

I thinks there are serious implications with this commit. I am not sure why 0.16 interprets 'touched' the way DeepSOIC described. Does that mean very feature object before 0.17 have to implement mustExecute() and explicitly check for each of their properties for touched() status? With wmayer's commit of reverting to the old meaning, any feature object that does not implement mustExecute() will break, and there are lots of them. And what's worse is that the breaking will be subtle, e.g. see here.

I think the new meaning of 'touched' is better. The object gets touched automatically if one of its property gets modified. Object does not need to explicitly check for every properties to ask for recompuation, and that's the behavior of mustExecute() before reverting. If any particular property needs special treatment, there are two solutions. 1) Use the Prop_Output on that property so that its change won't touch the object. 2) override mustExecute() to explicitly check for property changes. And I think 2) is a better way to handle the unnecessary recompute caused by Placement, by overriding GeoFeature's mustExecute(), v.s. the current solution, which demands every feature object to implement mustExecute().

[wmayer]

I thinks there are serious implications with this commit. I am not sure why 0.16 interprets 'touched' the way DeepSOIC described. Does that mean very feature object before 0.17 have to implement mustExecute() and explicitly check for each of their properties for touched() status?

Exactly! This was the initial design decision how things should behave and in a few places of the documentation of the classes DocumentObject and Document this was still explicitly said. But then JRiegel implemented the back-link mechanism (which ickby later prepared to be integrated into master) and therefore changed the implementation of the recompute of a document. I don't know if it happened on purpose or by accident but the meaning of the "touched" flag has suddenly changed which has led to a regression because many features had been unnecessarily recomputed which led to a waste of time and sometimes broken models.

But there was also an inconsistency with the implementation of v0.17. When e.g. manually marking an object for recompute then for several feature types it didn't show the overlay icon in the tree view because of incorrect implementations of mustExecute(). Furthermore, in the pre-release version of v0.17 these features wouldn't have been recomputed in several use cases and therefore ickby added the isTouched() check directly into Document::recompute() to somehow "solve" the inconsistencies. But after all this was only a hack and made the regression of superfluous recomputes even worse.


In the bug tracker there was a ticket where with a model with v0.16 it was possible to change a feature's placement in real-time (more or less) and with v0.17 it took a couple of seconds and even overwrote the manually set face colors due to a superfluous recompute.

Since I was not aware of any discussions about changing the initial design decision and the documentation still described the original behaviour I started to restore this behaviour which also fixed the above example with the placement.

With wmayer's commit of reverting to the old meaning, any feature object that does not implement mustExecute() will break, and there are lots of them. And what's worse is that the breaking will be subtle, e.g. see here.

If it's too much work to implement an appropriate mustExecute() for every single feature class one work-around is to implement a base class where the implementation of mustExecute() can also check for the "touched" flag.

If any particular property needs special treatment, there are two solutions. 1) Use the Prop_Output on that property so that its change won't touch the object. 2) override mustExecute() to explicitly check for property changes.

But when talking about efficiency then this way you don't cover all use cases because it's again the placement property where this doesn't work well. A feature whose placement is changed is able to adjust its appearance immediately because it doesn't have to modify its geometry but -- when talking about Part shapes -- only has to adjust the internal placement (TopLoc_Location in case of a shape) and for the visual representation it has to adjust the SoTransform node.
Setting the Prop_Output flag for the placement property to be still efficient is not an option because other features that depend on the feature that has changed its placement won't be recomputed and thus are wrong.

2) override mustExecute() to explicitly check for property changes. And I think 2) is a better way to handle the unnecessary recompute caused by Placement, by overriding GeoFeature's mustExecute(), v.s. the current solution, which demands every feature object to implement mustExecute().

I am not entirely sure if I got you right as you are a bit vague here. So your idea is to put back the "isTouched()" check inside DocumentObject::mustExecute and implement GeoFeature::mustExecute() where it is checked that if the only touched property is the Placement it will return 0. Is this what you meant?

[realthunder]

If it's too much work to implement an appropriate mustExecute() for every single feature class one work-around is to implement a base class where the implementation of mustExecute() can also check for the "touched" flag.

That's actually what I meant, to have a base mustExecute() return 1 if touch flag is set, and for GeoFeature to override this behavior by explicitly checking each of its property's touch flag, excluding Placement and those marked as Output.

On second thought, iterating over properties is inefficient too. Maybe we can add another property type similar to Prop_Output, so that the default behavior for each property on change is to set object's Touch and Enforce flag, but for properties with, say Prop_NoRecompute, it only set Touch but not Enforce flag. For Prop_Output type, it will not set either of the flags. In this way, object Touch flag can retain its old meaning, and it is easy to provide default mustExecute().

[wmayer]

That's actually what I meant, to have a base mustExecute() return 1 if touch flag is set, and for GeoFeature to override this behavior by explicitly checking each of its property's touch flag, excluding Placement and those marked as Output.

But I didn't mean DocumentObject as the base class but a basic feature class of a module which only serves an abstract interface for the more specialized feature classes.

On second thought, iterating over properties is inefficient too.

That's exactly the same concerns that I have too. If it were only used during a document recompute one could live with it but it's also permanently called by the tree view via a timer.

On second thought, iterating over properties is inefficient too. Maybe we can add another property type similar to Prop_Output, so that the default behavior for each property on change is to set object's Touch and Enforce flag, but for properties with, say Prop_NoRecompute, it only set Touch but not Enforce flag. For Prop_Output type, it will not set either of the flags. In this way, object Touch flag can retain its old meaning, and it is easy to provide default mustExecute().

I have to think about it in detail but this sounds like a good plan.

[wmayer]

Here we go: git commit 6186a310d and git commit 36b9a1c4