Thanks!
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...carlopav wrote: ↑Mon May 10, 2021 8:38 pm 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).
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.
Ok, so draftguitools.Move.proceed calls:matthijskooijman wrote: ↑Mon May 10, 2021 6:02 pm 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.
Code: Select all
groups.get_group_contents(self.selected_objects,
addgroups=True,
spaces=True,
noarchchild=True)
- 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)
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...)