Fix object reference arrows not hiding with per-object visibility#4455
Fix object reference arrows not hiding with per-object visibility#4455Aayushmaan-24 wants to merge 1 commit into
Conversation
Object reference arrows were correctly hidden when hiding an entire layer, but remained visible when toggling the visibility of individual objects. The root cause was in addRemoveObjectReferences(): it checked objectGroup->isHidden() to skip hidden layers, but never checked object->isVisible() for individual objects. Additionally, when a MapObjectsChanged event was emitted for a visibility toggle, only syncOverlayItems() was called (which only syncs positions), so the arrows were never removed. Fix: - In addRemoveObjectReferences(), skip invisible source objects and skip creating arrows that point to invisible target objects. - In changeEvent(), when MapObjectsChanged carries VisibleProperty, call addRemoveObjectReferences() to rebuild arrows. Fixes mapeditor#4451
bjorn
left a comment
bjorn
left a comment
There was a problem hiding this comment.
Thanks for trying to address this issue!
As-is, the change is not entirely complete and I think it's not a good idea to take target object visibility into account. See inline comments.
| forEachObjectReference(object->properties(), [&] (ObjectRef ref) { | ||
| MapObject *targetObject = DisplayObjectRef(ref, mMapDocument).object(); | ||
| if (targetObject && !targetObject->isVisible()) | ||
| return; |
There was a problem hiding this comment.
If we're going to hide based on target object visibility, it will be easier to do it in ensureReferenceItem. However, I'm not entirely sure it's helpful. We also don't hide arrows pointing at objects in hidden layers (only arrows pointing from hidden layers).
| mReferencesByTargetObject[targetObject].append(item); | ||
| }; | ||
|
|
||
| if (Preferences::instance()->showObjectReferences()) { |
There was a problem hiding this comment.
There's a check on object->isVisible() missing here.
| const auto &mapObjectsChange = static_cast<const MapObjectsChangeEvent&>(event); | ||
| if (mapObjectsChange.properties & MapObject::VisibleProperty) { | ||
| if (Preferences::instance()->showObjectReferences()) | ||
| addRemoveObjectReferences(); |
There was a problem hiding this comment.
This full refresh based on any toggle of visibility may be a little heavy. I think this is another reason to not base reference visibility on the target object, because in that case we could instead call the addRemoveObjectReferences overload that takes a specific object, for each of the changed objects only.
2 participants
Object reference arrows were correctly hidden when hiding an entire layer, but remained visible when toggling the visibility of individual objects.
The root cause was in addRemoveObjectReferences(): it checked objectGroup->isHidden() to skip hidden layers, but never checked object->isVisible() for individual objects. Additionally, when a MapObjectsChanged event was emitted for a visibility toggle, only syncOverlayItems() was called (which only syncs positions), so the arrows were never removed.
Fix:
Fixes #4451