Bug: draft move of link moves original object when MoveWithHost is true

[matthijskooijman]

So feel free to ask if I can help you in deciphering something I did or getting you more quickly into the discussion we had at that time.

Thanks!

just one hint: Draft and Arch barely take advantage of the more recent freecad developements that were carried on after the introduction of PartDesign. I'm thinking to expecially to App::GeoFeatureGroup and App::Link objects: they are objects that act on the representation (ViewProvider) rather than on the Shape/Placement attribute of the Object to calculate the position of the object on the scene (this can be very handy in architecture since you can change the position of a whole floor without having to recompute all the floor objects, cause the objects placements are not changed when you move the floor).

Right, I read about that indeed, though I haven't fully grasped it. AFAICS this works because e.g. Part has its own Origin that sets a new coordinate space, right? I can see how that would circumvent a whole lot of this stuff, but as Yorik already mentioned back there, does mean that objects do not know their own coordinates anymore (which might necessarily be bad, though). Also, it's really out of my league to consider how to make that work, let alone think about the migration path to not break every arch file out there...

Also take into account that Draft Move it's not the only way to move objects, so for example you could decide to move an Arch object with another workbench tool, vanishing any special algo that was contained into Draft Move.

I think we can fix this issue with the current approach, see below, by essentially moving all of that special algo into the placement onchanged handler, making draft move fairly dumb again.

Looking at where groups are handled, it seems that in a draft move, the GUI part *does* call get_group_contents on whatever thing you select after all and uses that for the actual move (so not just for ghosting), so maybe @carlopav's suggestion of simplifying get_group_contents would work (though there is already a `noarchchild=True` option passed, so maybe it is already simplified in the draft move code path). Have to look more closely later.

Ok, so draftguitools.Move.proceed calls:

Code: Select all

            groups.get_group_contents(self.selected_objects,                   
                                      addgroups=True,                          
                                      spaces=True,                             
                                      noarchchild=True)

Which means it will recursively return:
- All passed objects themselves
- The contents of DocumentObjectGroups, Space and Site objects (but not Drawing::FeaturePages) that are passed directly.
- Then recursively the contents of those types, and additionally Building and BuildingPart (because `noarchchild` is not passed to the recursive call, this is probably a bug, since now moving a group containing a BuildingPart moves all contained objects even without MoveWithHost, and doublemoves objects with MoveWithHost).

I guess DocumentObjectGroups, Space and Site are still relevant here, but for Space and Site, it might be better to give those the same onChanged handling for placement and do not treat them specially in the move tool.

The small downside of this is that the ghost while moving does not show the contents of those, even when they will be included in the move, but that problem already exists for other things (i.e. windows are not previewed in a move, though the openings are). But maybe this can be fixed by letting the move tool know about this new mixin class I'm suggesting (and/or duck type for a obj.Proxy.getMovableChildren() method) and then use a different object list for previewing than for the actual move, which has the upside of being consistent for all these arch-related moves. Note that for e.g. moving a Part, the preview is already handled, probably because Part has its own origin, so previewing the moved the part moves the origin which moves the contained objects or so.

I realized one more caveat: the onChanged approach currently only moves direct children, leaving it up to them to forward any placement (i.e. with a BuildingPart inside a BuildingPart, moving the outer moves the inner, which then moves its own children). However, DocumentObjectGroup does not do this, so a BuildingPart inside a group inside a BuildingPart willl break the chain. This can probably be fixed by (recursively) expanding groups in getMovableChildren.

So, here's the beginning of a plan.

• Fix the wall bug, so updating the placement of a wall moves its hosted objects.
• Move the onChanged Placement handling into mixin class (see below)
• Clean up placement propagation in onChanged to use matrices with @carlopav's commit.
• Stop using Draft.get_movable_children() in Draft.move (but probably leave the function in case third-party code uses it?), only move the objects passed.
• Let draftguitools.move expand only DocumentObjectGroups when selecting the objects to move, but include the result of getMovableChildren on each of these objects in the ghost (maybe recursively)

About the mixin class, I'd suggest naming that MovableChildrenMixin, which overrides onChanged and onChangedBefore to apply any placement changes to the movable children. The latter are defined by a getMovableChildren() method (like ArchComponent has now), which I think should include Group, Additions and Subtractions as far as available, and reverse Host/Hosts, provided those objects do not have MoveWithHost=false. This means a slight change from the current behavior (i.e. BuildingPart does not currently check for Host/Hosts), but I think this makes things more consistent and predictable. For Group/Additions/Subtractions, this should expand DocumentObjectGroups recursively before doing the MoveWithHosts check. Subclasses could also override this to add more if needed.

I thought about also refactoring get_group_contents, to give it a cleaner interface where you would simply specify all kinds of groups (groups, arch, walls, etc) that it should expand, defaulting to not expanding anything at all. However, it seems that this function is used in a lot of places and behaves fairly erratically (i.e. in get_windows(), it processes the entire OutList recursively, who knows what it will traverse and who depends on that), so I'm not confident I can probably refactor this without breaking a lot of things. So maybe just adding a new function with such a signature could be helpful (maybe just for DocumentObjectGroup initially), and others could slowly migrate to that.

Open questions:
- What to do with getHosts(). Rename to getHostedObjects()? Could we call it from getMovableChildren? Then it must move to the mixin, which is ok. Should it handle Group/Additions/Subtractions as well? getMovableChildren needs to distinguish between these, though. Are these really considered "Hosted objects", or just children or so? If so, why does MoveWithHost apply to them? Do we need better terminology here?
- Should the "placement propagation" in the new mixin become smarter with different objects (i.e. objects without placement)? It now just modifies the placement property, but the move tool does more things (like modifying Position instead of Placement on an annotation and similar stuff). One caveat on onChanged, you might no longer know whether you were moved or rotated, especially if you simplify this using matrices, but not all object types might be modifyable using matrices? Or maybe it would be consistent if all these arch objects only moved subobjects with Placement (as is already the case to some degree now), but it might be annoying if e.g. text or other annotations would stay behind when moving a buildingpart or so... Maybe Draft.move and Draft.rotate could be generalized into a Draft.transform (called by the former two) that could also be called by the MovableChildrenMixin? Added advantage is that move and rotate are equalized, since now move can handle more objects than rotate. Or alternatively, all these objects should get some "applyTransform" method that draft can just call and draft no longer has to know about all these object types, but I'm afraid that might become a bit of a mess (with all these C++ and python objects, and backward compatibility, and whatnot...)

[matthijskooijman]

In the concept of hosted objects, there's a few very specific exceptions:
- Roof objects subtracted (or added) from Wall objects, and all Window objects are not made invisible when modifying the wall's Subtractions (or Additions). Normally all additions and subtractions are made invisible.
- Rebar objects that have a Host property are not subtracted from their host (normally, all reverse Host and Hosts are subtracted).

In an attempt to handle these exceptions a bit more generically, and also clean up the handling of other hosted objects, I think that we can classify hosted objects in a few categories:

1. Simple contents (i.e. BuildingPart and Site). These use the Group attribute. These do not contribute to the host's (visible) shape (I would say, but its seems BuildingPart does build a collective shape based on its Group, Site does not), should render themselves, follow the host's visibilty, be claimed and normally be moved with their host (unless MoveWithHost is false).
2. Subtractions with their own identity (i.e. Roof subtracted from Wall). These contribute to their host's shape, but should also render themselves, not follow the host's visibility, not be claimed and not normally be moved along with the host (but they will have MoveWithHost=False normally)
3. Other additions and subtractions (i.e. a subtracted cube for an opening, or some shape added to a wall or so). These contribute to their host's shape, should not render themselves (i.e. be normally made invisible by their host), be claimed and should normally be moved with their host (unless MoveWithHost is false)
4. Objects that are contained inside the host (i.e. rebars). These use Host/Hosts. These do not contribute to their hosts shape (i.e. do not cut a hole), should render themselves, follow the host's visibility, be claimed if the ClaimHosted preference is set, and normally move with their host (unless with MoveWithHost=False, though that defaults to False for rebars, which is probably a bug?).
5. Objects that cut a hole in their host (i.e. windows). These use Host/Hosts. These behave identical to D, except that these additionally cut a hole in their host.

Most of these categories can be distinguished based on the attribute involved (i.e. Group, Additions/Subtractions, Host/Hosts), except for B vs C and D vs E. To distinguish those, currently object types are checked (i.e. Roof subtracted from Wall is B and Rebar is D), but maybe this could be made more generic somehow. For D vs E, an additional property "HostType" or "HostedType" or so could make sense, but that is a bit harder for B vs C, since there some Subtractions will be type B and some type C. So I'm not sure, but maybe the current hardcoded exceptions in code is fine (maybe centralized in a single getHostedObjects() method or so).

Something else to investigate, is how visibility propagation works. Currently, I think there's two versions of this:
- BuildingPart adds a ViewProviderGroupExtension which propages visibility to its Group
- ArchComponent.ViewProviderComponent has an onChanged which propages visibility to all objects that have a Host or Hosts property pointing to it.
- For Subtractions and Additions, no propagation is done, but ArchComponent does define a hideSubjects method that hides all additions/subtractions (except for roofs subtracted from walls) when additions or subtractions change (and the same for Mesh). Oddly enough this method is called in specific subclasses instead of just for all ArchComponents (and only in Wall, Structure, Window and Equipment, even though it seems all other ArchComponent subclasses can also use Additions and Subtractions?).

I've also noticed that a rebar hosted by a wall is *shown* when the wall is shown, but not *hidden* when the wall is hidden somehow, though I'm not sure why. I've also seen that the visibility property in the View tab and the icon in the treeview are not always updating properly (but also not consistently with each other, i.e. icon stuck at hidden and Visibility stuck at true, while pressing space does have the expected effect in the 3D view). Something iffy is still going on here.

[matthijskooijman]

Ok, so I ended up submitting a PR for the tiny fix in my original post, which is a targeted fix that extends the existing Clone exception to include Links and ensures that moving a window link no longer moves the original.

All of the other problems and issues I noted with MoveWithHost are still not addressed, but I really should be focusing on making a workable drawing and I can probably do without MoveWithHost for that, so I'll be postponing a fix for those for a while (unless it gets really in the way of my workflow :-p).

A few things to note for when I come back, though:
- Carlopav pointed out elsewhere that proper behavior of undo/redo with MoveWithHost is also a point of attention. I haven't checked how it behaves now, but I think I have not seen any exceptions in the code for when undo'ing, so it seems likely that an undo will undo the movement twice (first restoring the original placement of the hosted object, then the placement of the host object, which applies the same delta to the hosted object again). I haven't looked closely yet, but there seem to be some global "isRestoring()" state that is checked in other places, so something like that probably applies here.
- Reading back I noticed a bug reported linked by bitacovir earlier, about arch external reference not loading windows when referencing a wall. It's not strictly related to MoveWithHost, but it *is* another instance of a bug related to host/hosted relations, that probably needs to be included in this analysis somewhere.

[matthijskooijman]

Another thought for later: Maybe "MoveWithHost" is actually the wrong abstraction to achieve what we need here. It is rather Arch-specific and I wonder if the same thing could not be done by using more generic methods (i.e. AttachmentExtension). In a thread about paullee's intuitive arch placement, Cyclonit placed this comment along the same lines which I found well written and wanted to keep in mind here:

I am looking at this from a pure software engineering angle. It is always beneficial to create proper abstraction layers and to reduce complexity by reusing core systems. I am sure there is already some intermingling going on, but I have the feeling that the modular architecture of FreeCAD has ended up in a few suboptimal arrangements in this regard.

It is important to ensure that the differences between general assembly and arch are actual differences and not just different levels of abstraction. The goal of most assembly workbenches includes the attaching that your module can do plus additional features. From an assembly point of view, placing objects in rooms or features into walls is nothing more than a rigid assembly of two objects. Windows and doors causing holes in walls does not turn this into a different kind of problem. It only adds an additional layer on top. The placement should be solved through "assembly" methods, whereas the effects of the assembly can be dealt with upstream in the arch workbenches.

The same applies to the nature of objects themselves. Assembly deals with raw geometries. Take one part and join it to a different part based on some constraint on the relative geometries. Arch should expand upon this by adding its information to the objects, not by utilizing a completely different implementation of the objects themselves.

P.S.: I want to clarify that I don't want to offend anyone by saying this. I am by no means an expert of the internal going ons of FreeCAD so feel free to correct me on anything I got wrong :)

It does seem that paullee's work uses this approach (using AttachmentExtension to move attached objects when the "host" moves), but does add quite a layer of arch-specific stuff to calculate the AttachmentOffset when the host is changed.

Another observation is that the Arch workbench does currently work on slightly higher abstraction level, so it makes sense to just say "This window is hosted by this wall" and have the code figure out what that means, rather than the more generic and explict version where you would separately set up FreeCAD to 1) calculate how big the window is and cut such a hole in this wall 2) insert the window in the wall 3) attach the window to the wall so it moves when the window moves. So within that thought, having a MoveWithHost property and doing the moving implicitly could be sensible anyway...

[paullee]

Another thought for later: Maybe "MoveWithHost" is actually the wrong abstraction to achieve what we need here. It is rather Arch-specific and I wonder if the same thing could not be done by using more generic methods (i.e. AttachmentExtension). In a thread about paullee's intuitive arch placement, Cyclonit placed this comment along the same lines which I found well written and wanted to keep in mind here:

I am not following every details in the whole thread. Not sure if I understand. Are your suggest "MoveWithHost" should be changed? And what is your thought about Attachment approach ?

Thanks !

[matthijskooijman]

I am not following every details in the whole thread. Not sure if I understand. Are your suggest "MoveWithHost" should be changed? And what is your thought about Attachment approach ?

The line of reasoning is that "MoveWithHost" is actually an arch-specific concept, that essentially achieves the same thing that an AttachmentExtension does: Make sure that a hosted/attached object (e.g. window) moves with its host/support (e.g. wall). In the interest of reducing code and file format complexity, it could be considered to deprecate the MoveWithHost feature (rather than trying to fix it) and switch to using AttachmentExtension instead.

However, arguments for *keeping* MoveWithHost, is that it might provide a more integrated/semantical approach than with AttachmentExtension. For example, if you change the Hosts property of a Window, the Window will currently automatically start moving along with its new host. With AttachmentExtension, you would have to also update the Attachment properties. However, with proper UI support and/or changed handlers, this could probably be automated or abstracted away, of course.

Does that clarify?

As for how to use AttachmentExtension exactly in this context - I haven't really looked closely, I just wanted to consider the option for the future. Maybe AttachmentExtension should be expanded if it is missing features (at first glance, the "additional" transformation properties that you use in your ArchSketch work to calculate the AttachmentOffset suggest that maybe Attachment could be made more expressive so those additional Arch-specific properties are no longer needed. Again, I haven't looked too closely, though).

[carlopav]

Indeed substituting Arch MoveWithHost with the AttachExtension mechanism is something to experiment. It's strange we didn't think about that before... at least I didn't.
On the pro side there is the fact that it should be perfectly compatible with how objects are designed now.
On the cons side to me there is the fact that the hosted object placement is changed every time the host moves (that is also very compatible with current workflow)... but could this could be resources consuming when recomputing? I'm biased by being on the assembly workflow side

Edit: I once tried to implement a mechanism for the experimental wall to attach to an axis (that does not work good), that could be the right chance to try how hard is to extend the Attacher. here is the branch.

[paullee]

Does that clarify?

As for how to use AttachmentExtension exactly in this context - I haven't really looked closely, I just wanted to consider the option for the future. Maybe AttachmentExtension should be expanded if it is missing features (at first glance, the "additional" transformation properties that you use in your ArchSketch work to calculate the AttachmentOffset suggest that maybe Attachment could be made more expressive so those additional Arch-specific properties are no longer needed. Again, I haven't looked too closely, though).

Thanks, this is much clearer for me now

I have a discussion earlier whether AttachmentExtension would be expanded to fit 'Arch' use but I do not think so. For example, placing this Door at the Wall between Room A and Room B, 200mm from the end of Wall / corner of the Room. Seem this needs a 'layer' to interpret this to Attachment for it to happen. Currently, it is simple to calculate the AttachmentOffset for placing a Door and align it along the axis of the Wall. I hope the rest could be done later.

Maybe other people want to make something more interesting and useful ?