Bug fix for segfault iss #1795

[hant-hub]

This fixes a UI related segfault due to a stale observer reference in VTK. This was first found in issue #1795 where a python application segfaults if you delete and recreate the f3d engine.

[hant-hub]

For the test there is a thing where I believe due to memory layout issues the segfault is inconsistent. I could only get it to reliably detect the segfault by repeating the test multiple times. Currently I set that to 5 since that almost always catches the bug in my testing, but we might want to change that or install some kind of other memory validation.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.68%. Comparing base (2bf1c71) to head (5697d2b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1847   +/-   ##
=======================================
  Coverage   95.68%   95.68%           
=======================================
  Files         125      125           
  Lines        9885     9889    +4     
=======================================
+ Hits         9458     9462    +4     
  Misses        427      427           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hant-hub]

not really sure why the last two tests are failing, any advice would be appreciated.

[mwestphal]

Currently I set that to 5 since that almost always catches the bug in my testing, but we might want to change that or install some kind of other memory validation.

Thats fine, memory issues are like that :)

[Meakk]

not really sure why the last two tests are failing, any advice would be appreciated.

Not sure about the static analysis error, it seems unrelated.
About the crash with VTK 9.2.6, I can try to reproduce locally next week.

[mwestphal]

not really sure why the last two tests are failing, any advice would be appreciated.

VTK v9.2.6: Probably due to an already fixed bug in VTK, just not add the test with VTK 9.2.6 in the CMake logic

static_analysis: I dont get why this would be wrong and definitely not caused by ytour changes , is your branch based on master ?

[mwestphal]

nice fix! the main change looks good, small changes needed.

[snoyer]

The static analysis error is unrelated but it looks valid, the real question is why is it showing up only now?

library/src/interactor_impl.cxx:1049:66: style: Unused variable: doc [unusedVariable]
  std::vector<std::tuple<std::string, std::string, std::string>> doc;

f3d/library/src/interactor_impl.cxx

Lines 1041 to 1053 in 27d1e8a

	std::pair<std::string, std::string> interactor_impl::getBindingDocumentation(
	const interaction_bind_t& bind) const
	{
	std::vector<std::tuple<std::string, std::string, std::string>> doc;
	auto it = this->Internals->Bindings.find(bind);
	if (it == this->Internals->Bindings.end())
	{
	throw interactor_impl::does_not_exists_exception(
	std::string("Bind: ") + bind.format() + " does not exists");
	}
	auto docFunc = it->second.DocumentationCallback;
	return docFunc ? docFunc() : std::make_pair(std::string(), std::string());
	}

[mwestphal]

The static analysis error is unrelated but it looks valid, the real question is why is it showing up only now?

library/src/interactor_impl.cxx:1049:66: style: Unused variable: doc [unusedVariable]
  std::vector<std::tuple<std::string, std::string, std::string>> doc;

f3d/library/src/interactor_impl.cxx

Lines 1041 to 1053 in 27d1e8a

	std::pair<std::string, std::string> interactor_impl::getBindingDocumentation(
	const interaction_bind_t& bind) const
	{
	std::vector<std::tuple<std::string, std::string, std::string>> doc;
	auto it = this->Internals->Bindings.find(bind);
	if (it == this->Internals->Bindings.end())
	{
	throw interactor_impl::does_not_exists_exception(
	std::string("Bind: ") + bind.format() + " does not exists");
	}
	auto docFunc = it->second.DocumentationCallback;
	return docFunc ? docFunc() : std::make_pair(std::string(), std::string());
	}

Good point, moving this discussion to: #1848

[mwestphal]

(my bad, I did not intend to do that)

[mwestphal]

Anyway, static_analysis CI is fixed in master @hant-hub