<teepee>
ah, so it converts a list of signals to one signal with a value
<kintel>
I'll open a PR, but don't have a way of testing it right now
<teepee>
so we need mapped() for < qt6 and mappedInt for >= qt6?
<teepee>
I'll try it and we can merge, extra validation via snap then
<kintel>
same for the other signal; it was introduced in Qt-5.14
<InPhase>
kintel: The PR in question is using old-school SIGNAL and SLOT mechanisms which are unreliable. This should be swapped out for the post-C++11 function pointer connect calls.
<teepee>
reverting snap back for the time being
<InPhase>
kintel: Really the whole codebase should be using these calls, so that there is some compile-time checking going on.
<kintel>
InPhase do you know if there is a minimum Qt version we need to be aware of when using C++11 signals?
<teepee>
I'm relatively sure I've used them already, let me check
<kintel>
I
<InPhase>
kintel: It has been around for a while. I think qt5, yeah.
<kintel>
I've seen some usage in the codebase
<InPhase>
kintel: Note that if the stringly typed processing fails for those macros, you just get an error returned from connect at runtime which we are obviously not checking.
<InPhase>
kintel: The net result of this is that nothing happens when the signal is emitted, and things just silently fail.
<kintel>
yeah, moving those to build-time would be really good
<InPhase>
Off the top of my head I can't map how the commit in question causes the behavior reported, but the described problem issue sounded to me like SIGNAL/SLOT issues from before those function pointer methods arose, and in fact, I do see those being messed with in that commit.
<teepee>
that's one of those?
<InPhase>
connect(this->exportformat_mapper, SIGNAL(mappedInt(int)), this, SLOT(actionExportFileFormat(int))) ; I guess this is probably the failing call.
<InPhase>
Perhaps mappedInt exists on some platforms/versions but not others.
<teepee>
yes, due to mappedInt noe existing
<InPhase>
This really needs to be a build error.
<teepee>
yep!
<teepee>
we also use quite some auto-wire, that might be annoying to write down explicitely
mmu_man has joined #openscad
<teepee>
but it would safeguard against those silent failures
<InPhase>
I guess the outstanding question then is what Qt version one needs to flip mapped to mappedInt at to make that code correct. I assume mapped(int) disappeared, or else kintel wouldn't have mae that edit in the first place. :)
<InPhase>
s/mae/made/
<kintel>
InPhase Qt-5.15
<kintel>
mapped(int) was removed in Qt6
<InPhase>
Should be able to do then, I think: #if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
<InPhase>
Probably best to make it a function pointer as well at the same time. Converting the rest to function pointers could be a low hanging fruit for some eager beginner if you don't want to handle the pile. :)
<kintel>
teepee Are you certain that 3mfrendertest_issue1225 doesn't fail for you in debug mode on lib3mf V2?
<kintel>
I've chased it to createNefPolyhedronFromPolySet() not supporting PolySets with colors
<kintel>
..which is fine I guess; just remove colors in that case, but that should not be specific to CPU or OS
<teepee>
hmm, that's strange, I'll try again
drfff has joined #openscad
<kintel>
It _could_ be that quantization messes up the PolySet. That could potentially be behaving slightly different on different CPUs
<teepee>
I guess I can check via the github-ci script, that does certainly build differently as it showed the earlier Eigen issue but my normal build did not
<teepee>
or do I need to build with explicit -DDEBUG?
<kintel>
oh.. PolySet::quantizeVertices() only considers vertices and indices, but leaves color_indices alone
<kintel>
cmake -DCMAKE_BUILD_TYPE=Debug
<teepee>
I have -DCMAKE_BUILD_TYPE=RelWithDebInfo
<kintel>
not sure exactly what turns assertions off..
<InPhase>
kintel: The good news is, that compile-time check did its job on the mappedInt...
<teepee>
probably it does turn them off
<kintel>
InPhase nice
<InPhase>
kintel: The bad news is, 5.15.0 doesn't seem to be the right place to switch it. ;)
<kintel>
Uh?
<teepee>
the comments in PolySet say color_indices should be either empty or same size as triangles
<InPhase>
kintel: 20.04 failed on the PR.
<teepee>
it broke on Ubuntu 20.04 which should be older, I think it's not mappedInt
<teepee>
it does try mapped, but has some template instantiation error
<InPhase>
Oh, yeah. Some other issue.
<teepee>
it seems to have a problem with the function pointer not matching the enable_if for the template, but I may need a couple of days to get what it's trying to say ;-)
<InPhase>
Yeah. I see it. It's because mapped is overloaded, so a static_cast is needed.
<InPhase>
This is probably why mappedInt was created in the first place.
<InPhase>
kintel: So... I think: static_cast<void (*)(int)>(&QSignalMapper::mapped)
<teepee>
but why is the first enable_if saying: !IsPointerToMemberFunction
<InPhase>
I'm not sitting on a system old enough to test that.
<teepee>
you do, you could just comment that in the review and say "update" :-)
<teepee>
let me try that :)
<InPhase>
Or wait, that doesn't work for member functions, does it...
<teepee>
that's what the template fail said
<InPhase>
How does that magic work.
<teepee>
does it need a static function which gets the object manually for older compilers or something strange like that?
<kintel>
teepee yes, if color_indices is an empty array, there should be a single color which is used to color the entire PolySet. This is how color() works
<teepee>
aha, ok, that should be easy to test for once the PolySet is finished
<guso78k>
I am wondering, where PolySet::setColor is used ? Usual way to colorize something is to add a Color() node into the CSG tree ?
<kintel>
Btw., even if we fix the 3mf import, there is stlil a weakness in createNefPolyhedronFromPolySet() and possible other places. Not critical, but should be investigated, perhaps after merging the current work
<kintel>
guso78k yes, and the geometry evaluator calls setColor when encountering a color node
<guso78k>
kintel yes, that makes sense :) i am impressed by the C++ abilities in your paste (assign)
mmu_man has joined #openscad
<guso78k>
BTW: i am sorry for all this trouble caused by the File->Export Signal Mapping. But ultimately i think we will approach the better solution
<kintel>
guso78k I agree, refactoring is sometimes painful, but result is better
<kintel>
Our big weakness is lack of UI testing : /
<kintel>
teepee FYI: The PolySet assertion also fails for OFF imports
<kintel>
..but the OFF export in question doesn't write any colors, while the 3mf one does
<gbruno>
[github] kintel pushed 1 modifications (Don't use the mappedInt signal before Qt-5.15 (#5545) * Don't use the mappedInt signal before Qt-5.15 * Use C++11 signal connection * Cast for build error on Ubuntu 20.04 --------- Co-authored-by: Torsten Paul <Torsten.Paul@gmx.de>) https://github.com/openscad/openscad/commit/9454b4315b94445e65126a690f5a910130c7fd18