Page MenuHomePhabricator

Modifying the data does not modify the data node
Open, NormalPublic

Description

This issue is very close to T22322: Changing a renderer-specific property list of a data node does not modify the data node.
Also nodes are not marked as modified if the data is modified. This seems counter intuitive to me and breaks expectation. It seems that we implicitly draw the same conclusion when discussing T22322: Changing a renderer-specific property list of a data node does not modify the data node. But I want to ensure it explicitly that we think that "modify node if data is modified" is the correct behavior.

Event Timeline

I can only repeat that I think we made a huge mistake by propagating all these Modified() events from Properties to PropertyLists to DataNodes to BaseDatas and vice versa (well, partially as it seems when reading this task). It may made the recognition of modified input more easy for mappers but it is the cheating way of doing things instead of really listening explicitly to the objects one is interested in. For example, all these decisions made the Properties View very hard to use because as soon as a nested property is edited, the property tree view collapses as the model has to be regenerated due to a pseudo-modified DataNode. One could argue that this is not the most important thing currently but it clearly shows the ill-formed mechanism we introduced.

floca added a comment.EditedApr 15 2020, 6:15 PM

I can only repeat that I think we made a huge mistake by propagating all these Modified() events from Properties to PropertyLists to DataNodes to BaseDatas

OK. Then we shold thoroughly rediscuss it. Because this stands in contradiction to what we have decided/documented as solution in T22322.

and vice versa (well, partially as it seems when reading this task).

I wasn't aware that we have a modified event flow node -> data / propertylist -> property. If this is the case I also would deem it as a bug/design fault. This tasks also does not imply that. If so, I have written it badly. With this task I just wanted to assure that the decission we made for T22322 also covers "data -> node". So if data is modfied, the node also signals the modification.

It may made the recognition of modified input more easy for mappers but it is the cheating way of doing things instead of really listening explicitly to the objects one is interested in.
For example, all these decisions made the Properties View very hard to use because as soon as a nested property is edited, the property tree view collapses as
the model has to be regenerated due to a pseudo-modified DataNode. One could argue that this is not the most important thing currently but it clearly shows the ill-formed mechanism
we introduced.

First of all it just shows that there are illformed patterns/mechanismus that counterinteract. So we have a problem by design or missing help structures to use it easily. It is less clear what to blame, at least from my point of view.

I think we should carfully seperate between the essence of T22322 (that only complaints that render specific prop lists are handled in an other way than the general one; and this is a fair one. All prop lists should be handled the same way, what ever this way should be) and the question what is modified and how this is propergated or signaled.

Regarding the modification we also should distinct between two different aspects
A1: The indication of the modification and identifcation of outdatedness (this is what we use itk::ModifiedTime for).
A2: The signaling that something has changed to make it observable and we can react on (this is what we use the observers for).

The problem of PropertyView is given if A1 is solved badly and one cannot diseminate what really is outdated and therefor invalidates the complete model without need. Everything else is poor business logic (may be provoked by bad or missing support structures/ that help you to write the code easily).

The problem issued by T27308 and at several other places (e.g. touched upon in T22322) is about signalling (A2). So it is about being able to learn that something or a subpart have changed.
And there we also have several problems/issues in our code. The most prominent, from my point of view, can be boild down to: How to observe something you don't know (yet)?

  • This is also discussed in T22322; e.g.: We want to know if any rendering specific propertylist or element has changed, we cannot observe it directly as it is not there yet. So we need at least to tap in as soon as the number (added or removed) of property lists of a node or data changed (which is currently not possible as it is not monitored in the data node) to also start observing them.
  • This is also the problem of T27308. Because the only easy way to monitor changes in the DataStorage in general is on the node level. And currently that does not include changed data, but changed default property list ?!? (which breaks expectation).

In both cases we could solve A2 with micor observer management. But than I would strongly opt for offering a help structure (may it be integrated in often used interfaces or just to the disposal of the developer) that takes care of all that micromanagement. Handling the observers by hand is very error prone and causes a lot of dublicated code all over the place. So we might allow very precise "targeting", but we should always offer an easy to use possibility to do "broader" typical observations.

The same road I would go for A1. I think it is a "must have" that the scope of mtime of an class instance is as minimal as possible to realy identify what has changed (e.g. changes in a propertylist should not change the mtime of the data node; which does not mean that we cannot decide that changes in prop lists nevertheless triggers the emission of a modified event by the node (thats A2 not A1)). But then we should also have an easy posibility to query if something (included aggregated substructures) is outdated. E.g. virtual bool Class::IsOutdated(const itk::ModifiedTimeType& referenceTimePoint, bool includeSubElements = true) that you can call to query the up-to-date-state.

Hope the text was not to confusing. It was also in part brainstorming to sort my thoughts on the topic and to prepare/evolve the discussion.

floca added a comment.Apr 24 2020, 5:05 PM

Interim results of the discussion:

  1. It is a topic
  2. But it is to larga to discuss it in one metting. We should find dedicated slots for discussion or task owner how drives it.
  3. We should focus all design/rework considerations to the central data elements, namely:
    • DataNodes
    • BaseData and derived classes (and maybe agregated sub components)
    • PropertyLists
    • BaseProperty and derived classes
  4. And we should split the discussion between A1 and A2 (divide and conquere). I would start with A2.
  5. Regarding A1, it is also an option to think about which event (bus) mechanism is the best to use. (It must not be the build-in itk option; nevertheless there should be good options to don't use what is already there.)