set_autoscale_structures sets incorrect scale

[patsytau]

cube.off.txt test_polyscope.py.txt

This script, if installed with polyscope 1.3.1, correctly displays a cube on a grey grid. However, with 2.2.1, the window is blank, though it notes that the structure has been loaded.

Removing the polyscope.set_autoscale_structures(True) line results in displaying a cube on a grey grid, with the following output:

bbox diag = 10.392304845413264
gls = 10.392304420471191
gls = 10.392304420471191

With this line included, the window is blank, and the following output is given:

bbox diag = 10.392304845413264
gls = 10.392304420471191
gls = 0.009259259328246117

set_autoscale_structures(True) works in polyscope 1.3.1 as installed via pip.

[patsytau]

The same issue occurs with polyscope 2.3.0.

[patsytau]

Since this range is pretty large, I used the C++ library and bisected, it identified this commit as the first where the problem occurred:

commit cb3cc601adcd3edad19c11692762c950fd350d73
Author: Nicholas Sharp <[email protected]>
Date:   Tue Aug 22 20:50:15 2023 -0400

    implement weak handles for refs, slice planes to unique

 include/polyscope/polyscope.h   | 11 +++++---
 include/polyscope/weak_handle.h | 56 +++++++++++++++++++++++++++++++++++++++++
 include/polyscope/widget.h      |  4 ++-
 src/CMakeLists.txt              |  2 ++
 src/polyscope.cpp               | 39 ++++++++++++++++++++--------
 src/slice_plane.cpp             | 41 +++++++++++-------------------
 src/state.cpp                   |  4 +--
 src/structure.cpp               |  6 ++---
 src/weak_handle.cpp             | 11 ++++++++
 src/widget.cpp                  |  9 ++-----
 10 files changed, 129 insertions(+), 54 deletions(-)
 create mode 100644 include/polyscope/weak_handle.h
 create mode 100644 src/weak_handle.cpp

[nmwsharp]

Thanks for your patience on this, and for providing concrete steps and digging deep to find the commit. It sounds like there is likely an underlying bug, I will definitely look into it before the next release!

Unfortunately that commit did some low-level refactoring of how object lifetimes are managed, which indicates this may be a bad memory management / uninitialized access bug or something. All the more important to fix :)

[patsytau]

I've been debugging and I've just found the reason that this commit broke it :)

-float transScale = abs(glm::determinant(glm::mat3x3(T))) / T[3][3];
+float transScale = std::abs(glm::determinant(glm::mat3x3(T))) / T[3][3];

Prior to this line changing, it was calling abs(...) from the C library, but now with the std:: it's templated, and so it's calling the float version. So transScale (in this example) was taking a value of 0.0, so when determining the extent of existing structures in updateStructureExtents(), (the line state::lengthScale = std::max(state::lengthScale, x.second->lengthScale());) the call to x.second->lengthScale() was returning 0.0, so the value wasn't being updated.

I'll dig a little more, because the function being called now is obviously the correct behaviour, but something else is going on. Thankfully, it doesn't seem to be related to memory management, which is always a relief :)

[nmwsharp]

Fixed by #304