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

Enhanced markers #407

Closed
wants to merge 10 commits into from
Closed

Conversation

andresmmera
Copy link
Contributor

Hello all,

This PR modifies the existing code of the Marker class so as to allow the marker objects to work as delta markers. Additionally, it was modified the MarkerDialog class to let the user to decide de line width and the color of the markers.

It is not a big contribution, but I hope it helps ;)

Normal mode
selection_005
Delta mode
selection_006
MarkerDialog window
selection_007

P.S.: Originally, my idea was to include the markers in the dataset to perform math operations in the display window, like the ADS does,... but, at some point, I've realized that Qucs cannot handle equations after the simulation :(

@andresmmera
Copy link
Contributor Author

Uhm, Travis-CI crashed and left this message:
'No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself'
Does anyone know where the origin of the problem is? Is this related to the code?
On the other hand, I've noticed that AppVeyor is queuing every task but it seems to be quite stuck.

@ra3xdh
Copy link
Contributor

ra3xdh commented Jan 2, 2016

TravisCI crashes while running netlist test. To be certain, it cannot run test on DC_SW_bsim4v30pMOS_Ids_Vds_prj project. There is a problem while opening schematic with 3D-plots and markers. Please download Qucs test suite https://github.com/Qucs/qucs-test and try to open testsuite/DC_SW_bsim4v30pMOS_Ids_Vds_prj/bsim4v30pMOS_Ids_Vds.sch with your modified version of Qucs. It gives a Wrong 'diagram' line format! error and cannot open it. But master version operates on this schematic normally. So, there is a problem with the modified markers and 3D diagrams.

@andresmmera
Copy link
Contributor Author

Ok, thank you @ra3xdh, I've just found what's going on

@guitorri guitorri added this to the 0.0.20 milestone Jan 30, 2016
@ra3xdh ra3xdh mentioned this pull request Jan 29, 2017
11 tasks
@guitorri guitorri changed the base branch from master to develop February 18, 2017 12:52
@guitorri
Copy link
Member

I got this one rebased locally. I see you also performed a few Qt3Support fixes. I guess I will wait till #662 gets merged do rebase again.

@guitorri
Copy link
Member

Also, the color picker button can be the one from #672.

@guitorri
Copy link
Member

Is it absolutely necessary to create the [data set].Markers.dat file?
Shouldn't the diagram or marker objects be extended to accommodate such delta Markers?
I mean, this should be part of the schematic file somehow.
What about backward compatibility?
It might be interesting to have a visual feedback on the diagram indicating the delta. An arrow or a line. I need to thing about it.
Other than that, it works just fine. 👍

@felix-salfelder
Copy link
Member

felix-salfelder commented Feb 18, 2017

#314 ... perhaps with the refactored marker code, this will be much easier.

extra marker files will definitely fall on our feet when refactoring the data sources.

EDIT: #314 now seems to be #370

@in3otd
Copy link
Contributor

in3otd commented Feb 19, 2017

I did not actually test the code but I do not like that the line connecting the marker box and the graph has the same color as the graph line. The colored box around the marker already gives a visual cue about which marker belongs to which line and IMHO everything that has the color of a graph line inside a diagram must come from the data. Then one can think of pathological cases where the marker line superposes/interferes with the actual waveform shape (for rectangular/triangular waves, etc.). Opinions?

Extra bonus: it may be nice to have the delta markers with two lines, one to each point used for computing the delta; this will make immediately clear where it is actually measured. I guess this is what you were referring to above, guitorri?
And maybe a delta marker box should be black or multi-color, since potentially referring to different graphs (as in the example above)

@andresmmera
Copy link
Contributor Author

I did not actually test the code but I do not like that the line connecting the marker box and the graph has the same color as the graph line. The colored box around the marker already gives a visual cue about which marker belongs to which line and IMHO everything that has the color of a graph line inside a diagram must come from the data. Then one can think of pathological cases where the marker line superposes/interferes with the actual waveform shape (for rectangular/triangular waves, etc.). Opinions?

Good point. I didn't think about that case. Anyway, the default color can be changed.

Is it absolutely necessary to create the [data set].Markers.dat file? Shouldn't the diagram or marker objects be extended to accommodate such delta Markers?

Hum,... I didn't found another way to store the marker data, but I need to think about it.
By the way, I noticed that the Diagram.cpp and other classes are quite entangled... or at least they seemed entangled to me. Usually, relying on third party libraries is not a good idea... but, in the long term it could be interesting to replace the painting routines by the Qt charts module or even matplotlib or qcustomplot.

@felix-salfelder
Copy link
Member

replace the painting routines

that (among others) was the intent of the new marker code and the graph_deque. need to check what state it is in right now.

(iirc, the 2d plots are already using qt routines for the drawing, which was a fairly big lift)

@andresmmera
Copy link
Contributor Author

Hello,

I'm working again on this branch. I totally agree that it is not convenient to create a separate file to store markers: I'm thinking about a practical case of sharing a schematic by just copying & pasting it as raw text... Using an additional file to save the markers would force to share the markers file too and it would be quite annoying...

I'll think about an alternative solution...

@andresmmera
Copy link
Contributor Author

I need to do some cleanup, but so far:

  • I managed to get rid of those Marker.dat files
  • The line between the trace and the marker box is always black
  • It was added an extra line to the delta marker so as to give a visual clue of the reference marker
  • Backwards compatibility is guaranteed

@andresmmera
Copy link
Contributor Author

New appearance
selection_003

Backwards compatibiliy:

1. An old .dpl file can be loaded by this PR

Sheet created with the current Qucs code (master)
selection_001

Appearance of the previous file in this PR
selection_002

2. A .dpl file created with this PR can be loaded by the current Qucs code
Display sheet created with this PR
selection_004

Appearance of the previous sheet in the current Qucs (master)
selection_005

@in3otd
Copy link
Contributor

in3otd commented Apr 22, 2017

thanks!
On my machine changing the Color and Border width does not change the appearance, does it work there?
I tried to remove the marker name assigning an empty string but an empty line is shown in the marker box, is this normal/expected?
<picky mode on>Can we have the previous brownish color as default instead of black?<picky mode off>

I got it to segfault a couple of times opening an exiting Data Display but it doesn't happen all the time; try to open and close the Data Display in the project below a couple of times - here it crashes usually the first or second time.
markercrash.zip

I also notice a message on the console

Warning: QLayout: Attempting to add QLayout "" to SaveDialog "", which already has a layout

not sure if it was there before or it appeared with this PR.

@andresmmera
Copy link
Contributor Author

andresmmera commented May 1, 2017

On my machine changing the Color and Border width does not change the appearance, does it work there?

No, it was a bug... Now it is fixed

I tried to remove the marker name assigning an empty string but an empty line is shown in the marker box, is this normal/expected?

I didn't think about that... I've just added a check to as to avoid empty names. Thank you

Can we have the previous brownish color as default instead of black?

Of course, I've set the default marker color to 'Qt::darkMagenta'

I got it to segfault a couple of times opening an exiting Data Display but it doesn't happen all the time; try to open and close the Data Display in the project below a couple of times - here it crashes usually the first or second time.
markercrash.zip

I cannot reproduce that segfault here, but I noticed that when opening that .dpl file, the console output is filled with warnings like this:
dangerous: (maybe) overwriting control token60.2409

These warnings also appear in the master and develop branches. Maybe they're related to the segfault... or not

I also notice a message on the console
Warning: QLayout: Attempting to add QLayout "" to SaveDialog "", which already has a layout
not sure if it was there before or it appeared with this PR.

Uhm, I've seen that this also happens in the develop branch (but not in master). The warning is thrown just when you pick a qucs project from the projects tab. In this sense, I've been tracking it down and it seems to appear at 2e9f7d1
EDIT: Indeed the problem is at 2e9f7d1. I've just posted a comment there

@in3otd
Copy link
Contributor

in3otd commented May 2, 2017

thanks!
I can still see some segfault here; for the example above a gdb backtrace points to line 668 below, which I'm not sure is correct since the index nVarPos may point past the end of the vector but I did not really look at the whole code.

Thread 1 "qucs" hit Breakpoint 2, Marker::load (this=0x11d44c0, _s=...) at marker.cpp:668
668	    qDebug() << VarPos[nVarPos++];
(gdb) list
663	  do {
664	    j = n.indexOf('/', i);
665	    VarPos[nVarPos++] = n.mid(i,j-i).toDouble(&ok);
666	    if(!ok) return false;
667	    i = j+1;
668	    qDebug() << VarPos[nVarPos++];
669	  } while(j >= 0);
670	
671	  n  = s.section(' ',2,2);    // x1
672	  x1 = n.toInt(&ok);

Anyway, with another example the backtrace points to a call to ActiveMarkers.clear() in diagram.cpp

#0  0x0000000000423c5c in QBasicAtomicInt::deref() (this=0x0)
    at /usr/lib64/qt/include/QtCore/qatomic_x86_64.h:128
#1  0x0000000000423a1f in QString::~QString() (this=0x31ac9d0)
    at /usr/lib64/qt/include/QtCore/qstring.h:880
#2  0x0000000000609b3f in QMap<QString, std::vector<double, std::allocator<double> > >::freeData(QMapData*) (this=0x7fff3f7333b0, x=0x31ac620) at /usr/lib64/qt/include/QtCore/qmap.h:651
#3  0x000000000060864d in QMap<QString, std::vector<double, std::allocator<double> > >::~QMap() (this=0x7fff3f7333b0) at /usr/lib64/qt/include/QtCore/qmap.h:185
#4  0x000000000060870a in QMap<QString, std::vector<double, std::allocator<double> > >::clear() (this=0x31ac2f0) at /usr/lib64/qt/include/QtCore/qmap.h:446
#5  0x00000000005fc2ba in Diagram::paint(ViewPainter*) (this=0x31ac140, p=0x7fff3f733870)
    at diagram.cpp:141
#6  0x0000000000472829 in Schematic::drawContents(QPainter*, int, int, int, int) (this=
    0x31aa530, p=0x7fff3f733930) at schematic.cpp:427
#7  0x00007fd26c8c3fff in Q3ScrollView::drawContentsOffset(QPainter*, int, int, int, int, int, int) () at /usr/lib64/libQt3Support.so.4
#8  0x00007fd26c8c4257 in Q3ScrollView::viewportPaintEvent(QPaintEvent*) ()
    at /usr/lib64/libQt3Support.so.4
#9  0x00007fd26c8c5bc0 in Q3ScrollView::eventFilter(QObject*, QEvent*) ()
    at /usr/lib64/libQt3Support.so.4
#10 0x00007fd26dd20d76 in QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) () at /usr/lib64/libQtCore.so.4
#11 0x00007fd26d09346c in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
    at /usr/lib64/libQtGui.so.4

last debug messages on terminal were

Debug: ---------MARKERS MAP---------- 
Debug: Re{y}, Im{y}, x_position, y_position, indep_var, Delta_Mode? 
Segmentation fault (core dumped)

The segfault does not happen very often, usually when switching between Data Display tabs with lot of graphs

@@ -363,18 +496,24 @@ void Marker::paint(ViewPainter *p, int x0, int y0)
x2_ = p->drawText(Text, x0+x1+3, y0+y1+3, &y2_);
x2_ += int(6.0*p->Scale);
y2_ += int(6.0*p->Scale);
if(!transparent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the logic of 'transparent' was changed? Previously the marker border was always drawn and 'transparent' referred to the rectangle area, which was not transparent by default.
Now selecting 'transparent' removes the border and makes the rectangle area non-transparent.

@andresmmera
Copy link
Contributor Author

  1. Concerning the nVarPos, if I replace 'nVarPos++' by 'nVarPos-1', qucs crashes.
    I think that the segfault problem comes from here: 3374d90#diff-d6888ff24dbf42ac6c04ad6e983c02bcR665 By comparing this code with the master branch, I've noticed that I've accidentally added an extra autoincrement operator... This is equivalent to set nVarPos+=2 so... for sure this is the source of the problem...

  2. Well, I cannot remember the reason why I've changed the transparency logic. Probably, I was doing some kind of test and... I've forgotten to undo the changes. Sorry for that 😓

@in3otd
Copy link
Contributor

in3otd commented Jun 29, 2017

doing some more testing, found a little bug:

  • create a delta marker
  • change the name of the reference marker
  • double click on the delta marker... qucs crashes

I guess it looks/points to the old reference marker name and so something goes wrong

@in3otd
Copy link
Contributor

in3otd commented Jun 29, 2017

I also see that marker names with spaces do not work... I'll take a look at this.

@in3otd
Copy link
Contributor

in3otd commented Jul 2, 2017

I've fixed the problems above but some more general issues remain

  • when changing a marker name, if there is a delta marker using it as a reference the reference name it shows is not updated
  • similarly, if a marker used as reference is deleted this has no effect on the delta marker using it (I think it should actually change the marker type back to a normal marker.
  • same if the marker used as reference is changed from normal to delta, this has no effect on the other marker(s) using it as reference

need to think how all this can be handled

andresmmera added a commit to andresmmera/qucs that referenced this pull request Jul 9, 2017
This commit fixes the issues described by in3otd
(Qucs#407 (comment))

The fix consist on updating the ActiveMarkers map in the paint() event
so as to know if the reference marker has been removed or if its name
has changed. In these cases, the marker pointing to that reference is
set to be a convenitional marker.

In the same line, if the user activates the delta mode for the reference
marker, those markers pointing to that reference are converted into
conventional markers
@andresmmera
Copy link
Contributor Author

Hi,
@in3otd I've just pushed a fix for the issues you described.

https://drive.google.com/open?id=0BwDjrwGfd_z7WEdLNl81RXdVS0E

@in3otd
Copy link
Contributor

in3otd commented Jul 16, 2017

I've pushed some commits, one allows spaces in the Marker ID string: you may need to recreate/modify any existing schematic using Enhanced Markers as the marker ID is now enclosed in quotes in the schematic file.

Another thing I did is to keep the markers map in the diagram object only and not in every marker; I didn't see the need to have a copy in every marker and thought that only the markers parent needs to know all the markers characteristics.

There are some other points that in the long term may need to be addressed, e.g. now you can create delta markers between different graphs even if this does rarely make sense IMHO but nothing urgent at present.

andresmmera added a commit to andresmmera/qucs that referenced this pull request Jul 19, 2017
This commit fixes the issues described by in3otd
(Qucs#407 (comment))

The fix consist on updating the ActiveMarkers map in the paint() event
so as to know if the reference marker has been removed or if its name
has changed. In these cases, the marker pointing to that reference is
set to be a convenitional marker.

In the same line, if the user activates the delta mode for the reference
marker, those markers pointing to that reference are converted into
conventional markers
@andresmmera
Copy link
Contributor Author

andresmmera commented Jul 19, 2017

I've just pushed two commits. The first one fixes a bug: the reference marker combobox was being activated after placing three or more markers

Then after rebasing against develop, I made minor changes to extend the delta marker feature to the recently added phasor and AC temporal diagrams

@andresmmera
Copy link
Contributor Author

e.g. now you can create delta markers between different graphs even if this does rarely make sense

The last commit fixes this

andresmmera and others added 8 commits July 20, 2017 18:29
In this commit, the way the marker information (value, color and other
properties) was changed so as to avoid Marker.dat files which polluted
the file system and involved an unnecessary number of I/O file
operations.

In this sense, the Marker.dat table was replaced by a QMap structure in
'Diagram' class. This way, each diagram contains the necessary info of
its markers and it is updated when calling the paint() event. Moreover,
this table is passed to every marker in order to let 'markerdialog' know
what markers can be used as reference markers.

Regarding backwards compatibility, this code contains a legacy function
for loading the old marker format from the .dpl file.

Finally, it was added an extra (dotted) line to the delta markers so as
to give a visual clue about where the reference marker is.
* The marker format used until this commit was changed in order to
guarantee that a .dpl sheet with delta markers can be readed using old
versions of Qucs. Obviously, the old versions cannot handle the delta
feature, but at least the marker is loaded.

* On the other hand, it was found a segfault when doing ctrl+Z in Smith
Chart plots with delta markers. That happened because the data related
to the ref. marker was empty...
In this commit it was fixed a bug which prevented the marker from
updating the color and the width of the line.

Moreover, the default marker color was set to 'Qt::darkMagenta' and it
was added a check for preventing the user from removing the name of the
marker
This commit fixes an unfortunate alteration of the transparency logic.
Moreover, in order to try to fix the segfault issues noticed by in3otd,
it was removed an extra autoincrement in nVarPos...
This commit fixes the issues described by in3otd
(Qucs#407 (comment))

The fix consist on updating the ActiveMarkers map in the paint() event
so as to know if the reference marker has been removed or if its name
has changed. In these cases, the marker pointing to that reference is
set to be a convenitional marker.

In the same line, if the user activates the delta mode for the reference
marker, those markers pointing to that reference are converted into
conventional markers
and emit a warning if user tries to assign an emtpy string.
to avoid having to sync all the markers map copies in the markers.
This also fixes an unreported bug: after placing 3 or more markers,
the ref marker combobox was activated because improper initialization
@andresmmera
Copy link
Contributor Author

In line with #701 it was added a impedance/admittance selection box for those markers pointing at a Smith chart diagram

@andresmmera
Copy link
Contributor Author

I've applied the fix 428b687 here...

@guitorri
Copy link
Member

guitorri commented Oct 6, 2017

Testing.

@in3otd
Copy link
Contributor

in3otd commented Oct 6, 2017

IIRC from my latest testing, it was working fine - I was modifying the code to make, IMHO, a little straightforward; now the markers info are kept in the Diagram, moving that to the Graphs will automatically remove the possibility to create cross-graph delta markers, without the need to check on which Graph the marker is, since every Graph will have separate marker info, only for their own markers. But these modification can be done later.

@guitorri
Copy link
Member

guitorri commented Oct 6, 2017

I checked out the ImprovedMarkers branch.
Openned an existing .sch
Added a marker to a diagram and saved.
It opens fine on 0.0.19, but there is an extra pair of quotes being saved on the Rect diagram:

[master]~/git/qucs/qucs-test/testsuite/TR_sawtooth-discreet_prj $ git diff .
diff --git a/testsuite/TR_sawtooth-discreet_prj/sawtooth-discreet.sch b/testsuite/TR_sawtooth-discreet_prj/sawtooth-discreet.sch
index 5576391..97627de 100644
--- a/testsuite/TR_sawtooth-discreet_prj/sawtooth-discreet.sch
+++ b/testsuite/TR_sawtooth-discreet_prj/sawtooth-discreet.sch
@@ -1,4 +1,4 @@
-<Qucs Schematic 0.0.17>
+<Qucs Schematic 0.0.19>
 <Properties>
   <View=-4,-29,786,547,1.15104,0,0>
   <Grid=10,10,1>
@@ -17,7 +17,7 @@
 </Symbol>
 <Components>
   <_BJT T3 1 320 210 -81 -53 0 2 "pnp" 1 "1e-16" 1 "1" 1 "1" 0 "0" 0 "0" 0 "0" 1 "0" 0 "0" 0 "1.5" 0 "0" 0 "2" 0 "100" 1 "1" 0 "0" 0 "0" 0 "0" 0 "0" 0 "0" 0 "0" 0 "0.75" 0 "0.33" 0 "0" 0 "0.75" 0 "0.33" 0 "1.0" 0 "0" 0 "0.75" 0 "0" 0 "0.5" 0 "0.0" 0 "0.0" 0 "0.0" 0 "0.0" 0 "0.0" 0 "26.85" 0 "0.0" 0 "1.0" 0 "1.0" 0 "0.0" 0 "1.0" 0 "1.0" 0 "0.0" 0 "0.0" 0 "3.0" 0 "1.11" 0 "26.85" 0 "1.0" 0>
-  <Vdc V1 1 50 80 18 -26 0 1 "15 V" 1>
+  <Vdc V1 1 50 80 18 -26 0 1 "15 V" 1 "0 Ohm" 0>
   <GND * 1 50 110 0 0 0 0>
   <R R1 1 130 150 15 -26 0 1 "220 Ohm" 1 "26.85" 0 "0.0" 0 "0.0" 0 "26.85" 0 "european" 0>
   <GND * 1 190 480 0 0 0 0>
@@ -67,10 +67,11 @@
   <320 280 320 280 "OUT" 340 250 0 "">
 </Wires>
 <Diagrams>
-  <Rect 490 347 248 137 3 #c0c0c0 1 00 1 0 1e-05 3e-05 1 -0.380103 2 4.78441 1 -1 1 1 315 0 225 "" "" "">
+  <Rect 490 347 248 137 3 #c0c0c0 1 00 1 0 1e-05 3e-05 1 -0.380103 2 4.78441 1 -1 1 1 315 0 225 "" "" "" "">
        <"OUT.Vt" #0000ff 0 3 0 0 0>
+         <Mkr 1.78845e-05 208 -156 3 0 0 "Marker0" #800080 1 0 "" 0#0#0#0#0 0 0>
   </Rect>
-  <Rect 490 465 249 85 3 #c0c0c0 1 00 1 0 1e-05 3e-05 1 -7.22877 20 20 1 -1 2 1 315 0 225 "" "" "">
+  <Rect 490 465 249 85 3 #c0c0c0 1 00 1 0 1e-05 3e-05 1 -7.22877 20 20 1 -1 2 1 315 0 225 "" "" "" "">
        <"UTOP.Vt" #ff0000 0 3 0 0 0>
   </Rect>
 </Diagrams>

Any idea why?

@in3otd
Copy link
Contributor

in3otd commented Oct 7, 2017

um, I guess it actually comes from here

@guitorri
Copy link
Member

You guys are more familiar with the marker code.
Can you give it a try to rebase or merge develop into this?

Please document the latest marker line format in qucs-help.

@felix-salfelder
Copy link
Member

@guitorri need #370 first. #370 sanitizes the way graph data is stored/accessed/processed and how markers are attached to a graph. that particularly fixes moving around markers.

@Murmele
Copy link

Murmele commented Dec 5, 2017

Are there plans to write also the absolute values of the point where the delta mode marker is, to the label?

@andresmmera
Copy link
Contributor Author

Hum, I didn't t have that feature in mind, but if it doesn't overpopulate the text display I don't see much inconvenience on implementing that.
However, as @felix-salfelder said before, we should focus on #370 (btw, I'll try to help with that during this week).

@guitorri guitorri modified the milestones: 0.0.20, 0.0.21 Jul 25, 2018
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.

6 participants