Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor node #845

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Refactor node #845

merged 2 commits into from
Jul 26, 2024

Conversation

wawuwo
Copy link
Contributor

@wawuwo wawuwo commented Jul 16, 2024

Hi!

I've refactored the Node class to address the problem from #748.

@dsm seems to be working on the same issue in #832, but in a broader scope, I hope this piece doesn't interfere with his work. Anyway some synchronisation may be required upon merging (if it happens).

List of changes:

  • Remove dependency on Q3PtrList (most important)
  • Hide implementation details (in particular make private the container used to actually store connected elements)
  • Completely new API
  • Formatting and code structuring
  • Move 'Conductor' from element.h to a separate header file (not related to Node, just sorting things out)

I've done some tests on my side, in particular:

  • lay horizontal wire, lay vertical wire
  • lay two-segment (L, Г, etc.) wire
  • lay multiple-segment wire, i.e. chain of wires (click, move cursor and click, move cursor and click, and so on.)
  • connect wire to a component
  • start new wire from a middle of existing wire
  • end new wire in a middle of existing wire
  • move connected component around
  • delete connected wire segment, delete connected component

All described above went smoothly, but one cannot be 100% sure; some nasty bug is probably hiding waiting for perfect conditions to occur :(

- Remove dependency on Q3PtrList (most important)
- Hide implementation details (in particular make private
  container used to actually store connected elements)
- New class API
- Formatting and code structuring
@ra3xdh ra3xdh added this to the 24.4.0 milestone Jul 16, 2024
@ra3xdh ra3xdh mentioned this pull request Jul 16, 2024
@ra3xdh ra3xdh linked an issue Jul 16, 2024 that may be closed by this pull request
4 tasks
@dsm
Copy link
Collaborator

dsm commented Jul 16, 2024

@ra3xdh, @wawuwo, Hi, some changes related but my work not done properly, I just got back from holiday, I choose this way for Connections in node.h std::list<std::weak_ptr<Element> > Connections; we have too many raw pointer in qucs-s so we should use smart pointers for refactoring. We need to refactor thousands of lines of code :(

in sweepdialog.cpp I removed this lines not confusing for loop variables:

Node *pn;
Element *pe;

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 26, 2024

I have tested this and didn't found issues with schematic editing, save, load. I am merging this.

@ra3xdh ra3xdh merged commit 607dd68 into ra3xdh:current Jul 26, 2024
6 checks passed
@wawuwo wawuwo deleted the refactor-node branch July 26, 2024 15:06
@ra3xdh
Copy link
Owner

ra3xdh commented Aug 1, 2024

@wawuwo I have found a crash related to this PR. It is not possible to place the device on wire. Normal device connection works as expected. The placing of one device port on wire works as expected. Steps to reproduce:

  • Try to place any device over the wire. Usually device should be connected to the wire.
  • Application crashes at Schematic::insertComponentNodes

Could you have a look?

@wawuwo
Copy link
Contributor Author

wawuwo commented Aug 1, 2024

@ra3xdh,I am almost certain that the reason is that connected elements are being removed from node's internal std::list in deleteWire while iteration over this list (indirectly, through Node's begin() and end()) is in progress in insertComponentNodes, and it breaks the iterator.

I don't have a solution right now, need some time to think what could be done

wawuwo added a commit to wawuwo/qucs_s that referenced this pull request Aug 2, 2024
@ra3xdh ra3xdh mentioned this pull request Aug 2, 2024
4 tasks
@ra3xdh ra3xdh modified the milestones: 24.4.0, 24.3.1 Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of Q3PtrList wrapper
3 participants