Port idlist #429
Port idlist #429
Conversation
|
ah, the windows build env. in use doesn't support defaulting move constructors. |
|
Is this obsoleted by #433? |
|
no, #433 (ideally) builds on this. However, at least in my rebased version of this branch, neither one succeeds at the tests - this branch crashes, while the other fails tests and crashes. |
The issue before is that much of the code relies on a copy or assignment of IdList being a shallow copy, so using a shared_ptr was the simplest way to preserve that meaning. (Using a Vector directly was unsuccessful due to changed copy semantics. The variety of iteration methods makes a port to shared_ptr<unordered_map> harder and more far-reaching, if nevertheless worthwhile.)
|
OK, when I remove the merge-idlists patch from this branch, I do pass all the tests and can open something with a lathe object, though I get a double-free on shutdown. This is probably the best shot so far at such a port, but I'm not sure the value of it at this point since things break when I poke at stuff with varied ownership semantics. |
|
yeah it's unfortunately not easy at all |
phkahler
force-pushed
the
master
branch
2 times, most recently
from
dfb96de to
3f09eaf
Compare
|
@rpavlik Have you tested performance? There will probably be significant decrease. |
|
Why would it be expected to be slower? (Is MemAlloc a pool? I can't remember. If so, that makes sense.) I have no great allegiance here: if we somehow manage to avoid leaking/double-freeing/use-after-freeing memory with the current code, it's fine. Raw pointers owning memory just makes me anxious so I did something about it once. (and again, and again, when rebasing and testing ;) ) |
|
@rpavlik I am not sure, but it always worth testing before such significant changes. I said so, because almost every change of original containers by @jwesthues since 2.0 lead to performance decrease. std::things is not a very good at performance because should work everywhere and support all the data types. You can just look into id-list-indexing branch, the work I made on improvement of performace which is really significant, if you will test on really big examples like houses modelisation.slvs |
|
The idea could be revisited after the high-impact optimisations are in. But as it is, this PR isn't getting merged anytime soon, especially if it's unstable. There seems to be a battle of approaches in Solvespace, which originally was much more C-like. |
The project is from 2008 when Jonathan Westhues started it based on his SkethFlat and up to 2.0 in 2013 it was not even open source. And it was pretty much only him until 2015 when Whitequark and EvilSpirit joined. The code is updated/modernized from time to time - usually when there is another reason to touch it - as with the IdList in our case. |
Builds on #428 - replaces the manual memory management with something less manual. Had to go with a shared_ptr<vector> instead of just a vector because there's a lot of code that relies on assignment/copy being shallow. The tests pass here. (Unlike its related branch where I also ported List, that fails tests by being off by a few pixels. https://github.com/rpavlik/solvespace/commits/port-both )