use nested map for SDF names to save stack space. Fix #674 #676
Conversation
|
Now I'm wondering whether saving out the scenario will end up accessing every flag name index and populating it with enough empty strings to get the stack overflow anyway. |
NQNStudios
force-pushed
the
fix-stack-overflow
branch
from
84fdc5a to
2e05188
Compare
|
I thiiiink I got to all of the code paths that would fill the map with empty data, so now a scenario with no sdf names won't use memory to store blank strings. |
It wouldn't be a stack overflow even if it did – the map stores its data on the heap, not the stack. |
|
Wait a sec. I think my change is in line with the way SDFs themselves were already stored as an array2d of char. |
I'm not too sure what you mean by that, but… a 2-dimensional array doesn't specify which dimension is the rows and which is the columns. If anything, it's probably more common for the first index to be the row, because then it matches how you write out a 2-dimensional array literal in the source code. Though it looks like my array2d definition suggests the opposite was intended. Is that what you meant? Still, that x and y doesn't really mean anything. |
|
The documentation and the x and y on the typedef are consistent in implying the 350 is the columns in my opinion. But this is why I hate 2D arrays and if something is gonna reverse the coordinate order it should use r and c for the variable names imo. We can switch all of it to make it the other way around if that works better with the picker and the Set Many SDFs node. I just think my reasoning was correct. :P |
|
Yeah, I think the reasoning you used to come up with "350 columns and 50 rows" is correct. The old documentation says that, the new documentation says that, and the typedef implies it. And we could just run with it and not care that the SDF picker rotates the array. But it just seems like now is a good time to be strict about it. I'd like to explain why rotating it is better for the SDF picker. Because each item needs enough space for a string, the items are inevitably much wider than they are tall. As a result, I can fit twice as many rows as columns in the SDF picker. That means you have to scroll 35 times to get to the bottom, and 10 times to get to the far right. If it was done the other way around, you would need to scroll 70 times to get to the far right, but only 5 times to get to the bottom. |
|
That makes perfect sense. The documentation could include screenshots of the sdf picker to show the spatial relationship. |
NQNStudios
force-pushed
the
fix-stack-overflow
branch
2 times, most recently
from
2e05188 to
94c21f0
Compare
|
Okay. There's no way we can change the actual shape of the array2d for SDFs without breaking a bazillion things. So the change has to be semantic: instead of (x,y) coordinates, SDF flags now use (r,c) coordinates. This is explained in the documentation. And I added a TOC element for SDFs. |
src/scenedit/scen.sdfpicker.cpp
Outdated
| chosen_sdf.y = minmax(0, scenario.sdf_names[0].size() - 1, chosen_sdf.y); | ||
| viewport.x = cols * floor(chosen_sdf.x / float(cols)); | ||
| viewport.y = rows * floor(chosen_sdf.y / float(rows)); | ||
| // Note: x and y in chosen_sdf are actually (r,c) |
There was a problem hiding this comment.
This contradicts the other similar comments. Could that be the source of the crash you mentioned?
There was a problem hiding this comment.
Oh--I think this is definitely a mistake in my PR. But the crash I mentioned affects master.
Update semantics of the SDF array to (row, column) form to match the picker space Fix calref#674
NQNStudios
force-pushed
the
fix-stack-overflow
branch
from
1ddaa4f to
9b7df0e
Compare
This fixes the stack overflow and clarifies the spatial nature of the SDF grid.
Fix #675