Sketcher: exception from `MeasureGui::QuickMeasure` and `Part::TopoShape` interaction

[Andrei-Pozolotin]

Sketcher: exception from `MeasureGui::QuickMeasure` and `Part::TopoShape` interaction

[Syres916]

Doing a git bisect it was easy to prove the commit that fixed the issue: 374a663 specifically the line Delete="true" in /src/Mod/Measure/Gui/QuickMeasurePy.xml. Just remove it and recompile, the errors return for me.

[bgbsww]

Well, it appears that memory leak correction sidesteps the issue, which is still lurking.
From the top down, it is don't call Part::Feature::getShape with invalid input
From the bottom up it is make Part::Feature::getShape robust in the face of invalid input; return a null Shape instead of an exception.

The suspicion is that it was made less robust by the TNP work, and then triggered by the new QuickMeasure.

So while the proximate cause is the QuickMeasure changes, there is still an open question about the getShape call. There is prior art in other calls to getShape that are checking the return for null Shape, and disabled link visibility code in PartFeature.cpp ( around 970 and 875 ) that could be related.

Please don't close this issue until that is resolved.

[bgbsww]

Note that the ultimate exception is happening because getShape is getting called with either "V_Axis", "H_Axis", or "RootPoint" depending on what you clicked on. If you click on an actual shape or on nothing there doesn't appear to be an issue. This appears to be a mistake, since none of those things are a defined shape. The immediate caller is Measure::Measurement::findType and in general we are going into MeasureGui::QuickMeasure::onSelectionChanged even though there is no measurement being taken. The location of the exception may be a red herring. Certainly we need more info on the new code in measurement to understand if there is a part bug lurking here.

Part::TopoShape::getSubTopoShape TopoShapeExpansion.cpp:2031
Part::TopoShape::getSubShape TopoShape.cpp:343
Part::Feature::getSubObject PartFeature.cpp:157
_getTopoShape PartFeature.cpp:665
Part::Feature::getTopoShape PartFeature.cpp:1016
Part::Feature::getShape PartFeature.cpp:593
Measure::Measurement::findType Measurement.cpp:136
Measure::Measurement::addReference3D Measurement.cpp:100
Measure::Measurement::addReference3D Measurement.cpp:86
MeasureGui::QuickMeasure::onSelectionChanged QuickMeasure.cpp:87
Gui::SelectionObserver::_onSelectionChanged Selection.cpp:152
std::__invoke_impl<…> invoke.h:74
std::__invoke<…> invoke.h:96
std::_Bind::__call<…>(std::tuple<…> &&, std::_Index_tuple<…>) functional:420
std::Bind::operator()<…>(const Gui::SelectionChanges &) functional:503
boost::detail::function::void_function_obj_invoker1::invoke(boost::detail::function::function_buffer &, const Gui::SelectionChanges &) function_template.hpp:158
boost::function1::operator() function_template.hpp:763
boost::signals2::detail::call_with_tuple_args::m_invoke<…>(boost::function<…> &, boost::signals2::detail::unsigned_meta_array<…>, const std::tuple<…> &, boost::enable_if<…>::type *) const variadic_slot_invoker.hpp:105
boost::signals2::detail::call_with_tuple_args::operator()<…>(boost::function<…> &, const std::tuple<…> &, mpl::size_t<…>) const variadic_slot_invoker.hpp:90
boost::signals2::detail::variadic_slot_invoker::operator()<…>(const boost::shared_ptr<…> &) const variadic_slot_invoker.hpp:133
boost::signals2::detail::slot_call_iterator_t::dereference() const slot_call_iterator.hpp:110
boost::iterators::iterator_core_access::dereference<…>(const boost::signals2::detail::slot_call_iterator_t<…> &) iterator_facade.hpp:550
boost::iterators::detail::iterator_facade_base::operator*() const iterator_facade.hpp:656
boost::signals2::optional_last_value::operator()<…>(boost::signals2::detail::slot_call_iterator_t<…>, boost::signals2::detail::slot_call_iterator_t<…>) const optional_last_value.hpp:57
boost::signals2::detail::combiner_invoker::operator()<…>(boost::signals2::optional_last_value<…> &, boost::signals2::detail::slot_call_iterator_t<…>, boost::signals2::detail::slot_call_iterator_t<…>) const result_type_wrapper.hpp:64
boost::signals2::detail::signal_impl::operator()(const Gui::SelectionChanges &) signal_template.hpp:243
boost::signals2::signal::operator()(const Gui::SelectionChanges &) signal_template.hpp:722
Gui::SelectionSingleton::slotSelectionChanged Selection.cpp:581
std::__invoke_impl<…> invoke.h:74
std::__invoke<…> invoke.h:96
std::_Bind::__call<…>(std::tuple<…> &&, std::_Index_tuple<…>) functional:420
std::Bind::operator()<…>(const Gui::SelectionChanges &) functional:503
boost::detail::function::void_function_obj_invoker1::invoke(boost::detail::function::function_buffer &, const Gui::SelectionChanges &) function_template.hpp:158
boost::function1::operator() function_template.hpp:763
boost::signals2::detail::call_with_tuple_args::m_invoke<…>(boost::function<…> &, boost::signals2::detail::unsigned_meta_array<…>, const std::tuple<…> &, boost::enable_if<…>::type *) const variadic_slot_invoker.hpp:105
boost::signals2::detail::call_with_tuple_args::operator()<…>(boost::function<…> &, const std::tuple<…> &, mpl::size_t<…>) const variadic_slot_invoker.hpp:90
boost::signals2::detail::variadic_slot_invoker::operator()<…>(const boost::shared_ptr<…> &) const variadic_slot_invoker.hpp:133
boost::signals2::detail::slot_call_iterator_t::dereference() const slot_call_iterator.hpp:110
boost::iterators::iterator_core_access::dereference<…>(const boost::signals2::detail::slot_call_iterator_t<…> &) iterator_facade.hpp:550
boost::iterators::detail::iterator_facade_base::operator*() const iterator_facade.hpp:656
boost::signals2::optional_last_value::operator()<…>(boost::signals2::detail::slot_call_iterator_t<…>, boost::signals2::detail::slot_call_iterator_t<…>) const optional_last_value.hpp:57
boost::signals2::detail::combiner_invoker::operator()<…>(boost::signals2::optional_last_value<…> &, boost::signals2::detail::slot_call_iterator_t<…>, boost::signals2::detail::slot_call_iterator_t<…>) const result_type_wrapper.hpp:64
boost::signals2::detail::signal_impl::operator()(const Gui::SelectionChanges &) signal_template.hpp:243
boost::signals2::signal::operator()(const Gui::SelectionChanges &) signal_template.hpp:722
Gui::SelectionSingleton::notify Selection.cpp:424
Gui::SelectionSingleton::addSelection Selection.cpp:964
Gui::SelectionSingleton::addSelection2 Selection.h:339
SketcherGui::ViewProviderSketch::addSelection2 ViewProviderSketch.cpp:3737
SketcherGui::ViewProviderSketch::preselectToSelection ViewProviderSketch.cpp:4228
SketcherGui::ViewProviderSketch::mouseButtonPressed ViewProviderSketch.cpp:1007
Gui::ViewProvider::eventCallback ViewProvider.cpp:256
SoEventCallback::handleEvent(SoHandleEventAction*) 0x00007ffff1a06b47
SoAction::traverse(SoNode*) 0x00007ffff182f095
SoChildList::traverse(SoAction*, int, int) 0x00007ffff19c9908
SoGroup::doAction(SoAction*) 0x00007ffff1a10660
SoSeparator::doAction(SoAction*) 0x00007ffff1a2566c
Gui::SoFCUnifiedSelection::handleEvent SoFCUnifiedSelection.cpp:697
SoAction::traverse(SoNode*) 0x00007ffff182f095
SoChildList::traverse(SoAction*, int, int) 0x00007ffff19c9908
SoGroup::doAction(SoAction*) 0x00007ffff1a10660
SoSeparator::doAction(SoAction*) 0x00007ffff1a2566c
SoAction::traverse(SoNode*) 0x00007ffff182f095
SoHandleEventAction::beginTraversal(SoNode*) 0x00007ffff1838a9b
SoAction::apply(SoNode*) 0x00007ffff182f6e8
SoEventManager::actuallyProcessEvent(SoEvent const*) 0x00007ffff19de559
SIM::Coin3D::Quarter::QuarterWidget::processSoEvent QuarterWidget.cpp:1028
SIM::Coin3D::Quarter::SoQTQuarterAdaptor::processSoEvent SoQTQuarterAdaptor.cpp:752
Gui::View3DInventorViewer::processSoEventBase View3DInventorViewer.cpp:2571
Gui::NavigationStyle::processSoEvent NavigationStyle.cpp:1493
Gui::TouchpadNavigationStyle::processSoEvent TouchpadNavigationStyle.cpp:275
Gui::NavigationStyle::processEvent NavigationStyle.cpp:1464
Gui::View3DInventorViewer::processSoEvent View3DInventorViewer.cpp:2566
SIM::Coin3D::Quarter::EventFilter::eventFilter EventFilter.cpp:167
QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) 0x00007ffff252fb9a
QApplicationPrivate::notify_helper(QObject*, QEvent*) 0x00007ffff301f702
QApplication::notify(QObject*, QEvent*) 0x00007ffff3027364
Gui::GUIApplication::notify GuiApplication.cpp:83
QCoreApplication::notifyInternal2(QObject*, QEvent*) 0x00007ffff252fe3a
QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer&, bool, bool) 0x00007ffff3025e47
0x00007ffff307bd40
0x00007ffff307efd5
QApplicationPrivate::notify_helper(QObject*, QEvent*) 0x00007ffff301f713
Gui::GUIApplication::notify GuiApplication.cpp:83
QCoreApplication::notifyInternal2(QObject*, QEvent*) 0x00007ffff252fe3a
QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) 0x00007ffff2916307
QWindowSystemInterface::sendWindowSystemEvents(QFlagsQEventLoop::ProcessEventsFlag) 0x00007ffff28eba2c
0x00007fffebbe2d6e
g_main_context_dispatch 0x00007ffff015bd3b
0x00007ffff01b1258
g_main_context_iteration 0x00007ffff01593e3
QEventDispatcherGlib::processEvents(QFlagsQEventLoop::ProcessEventsFlag) 0x00007ffff25890b8
QEventLoop::exec(QFlagsQEventLoop::ProcessEventsFlag) 0x00007ffff252e75b
QCoreApplication::exec() 0x00007ffff2536cf4
tryRunEventLoop Application.cpp:2042
runEventLoop Application.cpp:2062
Gui::Application::runApplication Application.cpp:2139
main MainGui.cpp:278
__libc_start_call_main 0x00007ffff1e28d90
__libc_start_main_impl 0x00007ffff1e28e40
_start 0x000055555557e005

[Andrei-Pozolotin]

@maxwxyz: it is a critical bug, rather than a feature: it makes the sketcher unusable

[Syres916]

This is the change I made locally main...Syres916:FreeCAD:Measure_Exclude_Sketch_Edit_Axes_Root_Point but it's a fudge, IMHO Quick Measure shouldn't be available at all in Sketch Edit Mode.

@bgbsww it's my understanding that one workbench's Gui cannot access another's Gui header files such as QuickMeasure trying to check isSketchInEdit(doc) in Mod/Sketcher/Gui/Utils.h. I did try it and failed hence applying the fudge locally.

[bgbsww]

@PaddleStroke maybe?

[PaddleStroke]

I think MeasureGui can know about SketcherGui. However that would create a dependency on SketcherGui which is IMO not necessary nor ideal.
Besides there are other cases where this could happen I think. i'll check it out. However currently I cannot reproduce the crash as I am not able to select the root object, probably due to the bug. But that should not prevent me from fixing this.

[PaddleStroke]

So the bug is not on Measure, it is doing things correctly:

• You select something it is added in the current references.
• Then findType is determining the type of measure of measure to do based on the selected references.
• If the reference does not have a valid shape, getShape should return a null shape in which case the type becomes invalid and no text is shown in the quickmeasure label.

            TopoDS_Shape refSubShape;
            try {
                refSubShape = Part::Feature::getShape(*obj,(*subEl).c_str(),true);
                if(refSubShape.IsNull()){
                    return MeasureType::Invalid;
                }
            }

The bug is coming from PartFeature.cpp.

[PaddleStroke]

I had a look but this is quite complex so @bgbsww could you take a look at this bug? It basically happen when you call getShape with a sketch object and subname 'RootPoint'

Part::TopoShape::getSubTopoShape TopoShapeExpansion.cpp:2031
Part::TopoShape::getSubShape TopoShape.cpp:343
Part::Feature::getSubObject PartFeature.cpp:157
_getTopoShape PartFeature.cpp:665
Part::Feature::getTopoShape PartFeature.cpp:1016
Part::Feature::getShape PartFeature.cpp:593

[Andrei-Pozolotin]

Update: with build at revision=37243, with delta=37243-37227=16 commits - the issue is gone.

git log --name-status --max-count=16 > git.log shows no obvious change that could have resolved this:

git.log

commit 0095e52466c79c2135fce860a341554f3f51156c
Author: bgbsww <[email protected]>
Date:   Thu May 9 07:01:31 2024 -0400

    Support saving the tnp recalculated file in profile testing

M	src/Mod/Test/CMakeLists.txt
M	src/Mod/Test/TestPerf.py
M	tools/profile/perftest.sh

commit 2f9b97ce3abcef61c266396a2c8b2fd51d36edd2
Author: Roy-043 <[email protected]>
Date:   Thu May 9 11:12:19 2024 +0200

    Draft: Add "legacy importer only" to DXF option (#13638)
    
    * Draft: Clarify "legacy only" DXF options in the preferences
    
    Fixes #13598.

M	src/Mod/Draft/Resources/ui/preferences-dxf.ui

commit 3bb6f92885021c5a5667c728486eaf6f8d515524
Author: Jacob Oursland <[email protected]>
Date:   Tue May 7 16:56:54 2024 -0600

    Conda: add libspnav to linux builds.

M	conda/environment.devenv.yml

commit 70be39dd0d143277f236658781ca1ad639505776
Author: wmayer <[email protected]>
Date:   Wed May 8 18:36:28 2024 +0200

    Gui: Fix PythonWrapper::toEnum() to also accept a Python int as argument
    
    See https://github.com/FreeCAD/FreeCAD/pull/13611#issuecomment-2097126565

M	src/Gui/PythonWrapper.cpp

commit 7e2bbccf22e52a1a69e1732dd58aec79fc570e6c
Author: Jacob Oursland <[email protected]>
Date:   Tue May 7 18:04:58 2024 -0600

    Conda: pin zstd==1.5.6 to fix Windows CI failures.

M	conda/conda-env.yaml
M	conda/environment.devenv.yml

commit 82101ac89049eea6e89a9c57c83ccf5282a3c03c
Author: wmayer <[email protected]>
Date:   Wed May 8 16:53:29 2024 +0200

    Part: Fix memory leaks

M	src/Mod/Part/App/PropertyGeometryList.cpp
M	src/Mod/Part/App/PropertyTopoShapeList.cpp
M	src/Mod/Part/parttests/TopoShapeListTest.py
M	src/Mod/Part/parttests/TopoShapeTest.py

commit 374a663d2cbf15443a71779cdb2db93724ccd823
Author: wmayer <[email protected]>
Date:   Wed May 8 14:35:14 2024 +0200

    Measure: Fix memory leaks

M	src/App/MeasureManager.cpp
M	src/Mod/Measure/Gui/QuickMeasurePy.xml

commit 8742c371317f3f068f701929d24f252f3665dc98
Author: wmayer <[email protected]>
Date:   Wed May 8 11:29:23 2024 +0200

    Material: Fix memory leaks

M	src/Mod/Material/App/Array2DPyImpl.cpp
M	src/Mod/Material/App/Array3DPyImpl.cpp
M	src/Mod/Material/App/MaterialManagerPyImpl.cpp

commit 8941b941ab48c6b495e346b10a060743dbd776ab
Author: Furgo <[email protected]>
Date:   Wed May 8 04:57:19 2024 +0000

    Change property view panel title to sentence case

M	src/Gui/PropertyView.cpp

commit 1889dd2f7bce28dd292f2c0b249a64bcd1213266
Merge: 40b9c972c5 71d96bccac
Author: Chris Hennes <[email protected]>
Date:   Tue May 7 14:19:01 2024 -0500

    Merge pull request #13867 from bgbsww/bgbsww-toponamingFixBadPad
    
    Toponaming/Part: Fix fillet/pad bug, add tests

commit 40b9c972c5dd4a7d3837d3b1306ec7b6e88a7338
Author: wmayer <[email protected]>
Date:   Tue May 7 16:00:12 2024 +0200

    Assembly: Fix warning: -Wunused-variable

M	src/Mod/Assembly/App/AssemblyObject.cpp
M	src/Mod/Assembly/App/PreCompiled.h
M	src/Mod/Assembly/Gui/PreCompiled.h
M	src/Mod/Assembly/Gui/ViewProviderAssembly.cpp

commit 2aa96bee98a8e425a64e2099484e90e8e37ddcd5
Author: wmayer <[email protected]>
Date:   Tue May 7 15:42:03 2024 +0200

    Core: Refactor several methods of ToolBarManager

M	src/Gui/ToolBarManager.cpp
M	src/Gui/ToolBarManager.h

commit fdc78799b6e2a0c7fd3d8758a0d028b6b4836a1e
Author: wmayer <[email protected]>
Date:   Tue May 7 14:41:39 2024 +0200

    Core: Fix linter warnings

M	src/Gui/ToolBarManager.cpp

commit e86272fab276c4e33222ff16b92b7b180f897673
Author: wmayer <[email protected]>
Date:   Tue May 7 14:08:42 2024 +0200

    Core: Fix build failures and warnings

M	src/Gui/PythonWrapper.cpp
M	src/Gui/ToolBarManager.cpp
M	src/Gui/ToolBarManager.h

commit 71d96bccaca0ecb11f74063843b6ab0a99a3049e
Author: bgbsww <[email protected]>
Date:   Tue May 7 11:50:08 2024 -0400

    Fix tests, slip in TNP defense

M	src/App/ComplexGeoData.cpp
M	src/Mod/Part/App/PartFeature.cpp
M	src/Mod/PartDesign/PartDesignTests/TestTopologicalNamingProblem.py
M	tests/src/Mod/Part/App/TopoShapeExpansion.cpp

commit c1bc3b6e9a404ce68e4480711aba1972a12241f3
Author: bgbsww <[email protected]>
Date:   Tue May 7 08:17:44 2024 -0400

    Update ToolBarManager.cpp for QT 6.7
    
    Replace obsoleted method call

M	src/Gui/ToolBarManager.cpp

@bgbsww :

• please verify at revision=37243?
• did that resolve the issue for you?
• can you see the change that resolved?

[Andrei-Pozolotin]

@Syres916 : thank you!

@PaddleStroke, @bgbsww, and the verdict is ...

gremlins?