LibreCAD icon indicating copy to clipboard operation
LibreCAD copied to clipboard

Application crashes after opening file and mousing over drawing

Open Daeraxa opened this issue 10 months ago • 4 comments

Version: 2.2.2.3-alpha
Compiler: Microsoft Visual C++
Compiled on: Apr 23 2025
Qt Version: 6.9.0
Boost Version: 1.87.0 System: Windows 10 Version 22H2

Made a file based on my normal template just to try out some of the new changes but the application keeps crashing when interacting with it.

Steps:

  • Open LibreCAD and click away errors from https://github.com/LibreCAD/LibreCAD/issues/2127
  • File > Open > Untitled.dxf - Untitled.zip
  • Mousing over any of the lines in this drawing causes LibreCAD to crash. Can zoom and pan as normal but interacting with a line crashes. Nothing shows in the debug log window but can provide further info if I can be told how.

Daeraxa avatar Apr 25 '25 19:04 Daeraxa

@Daeraxa Oh, that's really interesting dxf - thank you for sharing it. I was able to reproduce a crash on hover - but I'm sure it will be fixed in the next update.

sand1024 avatar Apr 25 '25 22:04 sand1024

@dxli well, actually the issue was not related to the ownership in larger sense and I'm not sure that it worth to change the existing ownership approach...

The root issue was much simpler - in RS_DimAligned, the call to detach() was mistakenly removed when you've made changes related to entity's id. If call to detach is restored (as for other entities) - everything works fine.

sand1024 avatar Apr 28 '25 22:04 sand1024

@sand1024

Please feel free to fix the detach issue.

My fix is more about the data model of rs_entitycontainer.

Rule of 5: we had a dtor and a copy ctor for RS_EntityContainer, but no copy asigner, or move ctor/asigner. Even if everything works now, this opens the possibility for future bugs. The fix is to make sure RS_EntityContainer is copied/moved properly.

Correctness is better ensured at the data model level before the algorithm levels.

dxli avatar Apr 29 '25 01:04 dxli

@dxli Yes, I've addressed that fix in PR #2131 already (not merged yet). Yet as far as I can see now after the merge it might be less relevant (as clone is implemented differently).

as for rule 5 - yes, in general it's nice to follow, yet full support of it may be over complicated and actually not needed for most of domain specific classes. For example - I can hardly imagine that, for example, copy assigner is necessary for container or so.

In most cases, all operations with entities are either assignments of pointers or creation of clones. Thus while in theory a full support of rule of 5 is nice, in practice it may just add a dead code that is hardly used.

So at the end the impact of full support of rule 5 on the codebase it's rather neutral or slightly positive - just some additional protection for the case where someone, somewhere and somehow will use entities in unusual way (that most probably will be just a developer's bug).

sand1024 avatar Apr 29 '25 10:04 sand1024