file: rework saving and loading for correctness and to avoid copying #1538
Conversation
Rework the object attributes to avoid needlessly iterating over all of them when needing to save a specific object or load a specific attribute of an object. While at it, add type verification for member serialisation. This caught an issue with the `Group::forceToMesh` member, which was incorrectly serialised as an `int`, rather than as a `bool`. Also change the file saving logic so that it doesn't have to go through the `sv` member of `SolveSpaceUI`, which needlessly requires copying objects over and over. This leaves `sv` to be used only as a temporary during file loading, but there's no reason to keep it as a member for this, so it is now made a local variable in the two functions that need it.
|
@rpavlik What do you think of this? I'm a huge anti-fan of templates, closures, and other "advanced" C++ features. It just seems overly complicated. This makes the code less maintainable by me, but I try to focus on other areas like geometry anyway. I like the part about sv becoming local ;-) What do you think of this change overall? |
|
I'd also like to hear what @rpavlik thinks. of this. In my opinion this is mostly syntactic sugar - makes the code longer and less readable.
|
My motivation for doing it is mainly to avoid the copy needed during save, which with my work on reworking
For now it is indeed harmless in practice, but if a member which has an alignment requirement that is less than |
Rework the object attributes to avoid needlessly iterating over all of them when needing to save a specific object or load a specific attribute of an object.
While at it, add type verification for member serialisation. This caught an issue with the
Group::forceToMeshmember, which was incorrectly serialised as anint, rather than as abool. Thankfully this didn't overwrite the next member because there are padding bytes in between, but we shouldn't continue to rely on this broken behaviour.However, this change unfortunately breaks the tests, since the save logic skips int members which are set to 0, but not boolean members which are set to
false. This now causes theGroup::forceToMeshmember to be emitted into the file where it previously wasn't. If we add the same logic to boolean as well, this would still break the tests because other boolean values were previously serialised. So we need to update the tests either way, but I'm not sure if I should just change the existing files, or back them up with_v31added to their name, like the other ones which have_v20and_v21. @ruevs, @phkahler please advise on the way to do it, and if I should change the boolean serialisation logic to match the one used for ints, and skip members set tofalse.Also change the file saving logic so that it doesn't have to go through the
svmember ofSolveSpaceUI, which needlessly requires copying objects over and over.This leaves
svto be used only as a temporary during file loading, but there's no reason to keep it as a member for this, so it is now made a local variable in the two functions that need it.