Graph rework rebased

[andresmmera]

This PR is the result of rebasing ba8c29b57343df5b2f787a5f879cce931ac5b00f against develop. In addition to the issue I pointed out here, the build with autotools crashes (the build with CMake is fine) and there are some warnings there... I'll take a look as soon as possible

[felix-salfelder]

thanks alot Andres.

reverting to the develop Makefile seems to fix the build. https://github.com/felix-salfelder/qucs/tree/andresmmera-GRAPH_REWORK_REBASED

(the relevant/useful changes were already merged in some other context).

[guitorri]

Reverted the Makefile.

For the record, Travis is marking linux-clang-autotools and osx-clang-autotools as a pass, but both fail with: make[4]: *** No rule to make targetqucs.qrc', needed by qrc_qucs.cpp'. Stop.

[guitorri]

New build issue:

cc1plus: warning: unrecognized command line option "-Wno-deprecated-register" [enabled by default]
libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I../../../../qucs/qucs/diagrams -I../.. -I/usr/include/qt4 -I/usr/include/qt4/Qt -I/usr/include/qt4/QtGui -I/usr/include/qt4/QtCore -I/usr/include/qt4/QtSvg -I/usr/include/qt4/QtXml -I/usr/include/qt4/QtScript -I/usr/include/qt4/Qt3Support -DQT_NO_DEBUG -DQT_NO_DEBUG_OUTPUT -DQT3_SUPPORT -DQT3_SUPPORT_WARNINGS -DQT_DEPRECATED_WARNINGS -DQT_THREAD_SUPPORT -D_REENTRANT -I../../../../qucs/qucs -g -O2 -pipe -fno-exceptions -Wno-deprecated-register -std=c++0x -c ../../../../qucs/qucs/diagrams/diagramdialog.cpp -o diagramdialog.o >/dev/null 2>&1
../../../../qucs/qucs/diagrams/diagram.cpp:39:18: fatal error: main.h: No such file or directory
 #include "main.h"
                  ^
compilation terminated.

It is related to the cleanup we did on main.cpp. There is main.h no more... I will push a fix

[in3otd]

For the record, Travis is marking linux-clang-autotools and osx-clang-autotools as a pass, but both fail

I think we saw this already, and it's of course quite bad. I think it's due to the fact that Travis does not look at the status of each command but only at the final status, so if the last commands returns pass everything seems fine - or probably something like this

[in3otd]

I've opened #776 as a reminder for the Travis issue

[guitorri]

@andresmmera I cannot reproduce the issue you describe below. Can you share your testcase?

However, I found a bug coming from ba8c29b (or even before). Please take a look at this https://drive.google.com/file/d/1WJ5XZB5geX-JvyYiXEKJ5sIQTHlfYkXc/view, I guess this is related to the macro in marker.cpp, but I need to take a closer look.

[andresmmera]

Unfortunately, I've accidentally removed that schematic a few weeks ago...

However, the issue appeared when the marker took values outside of the diagram limits. I've just tried to reproduce the same scenario with a different schematic, but I cannot see that bug because it now seems not to place markers at graphs that are partially above/below the y-axis limits.

This sample schematic I'm using now for debugging this stuff (I've took that from the Qucs examples): https://gist.github.com/andresmmera/a4ec13643820a0f566a26240eb9bf3df

There are two diagrams at the bottom of the schematic where I plot the same variable. In the diagram at the left, I've set the y-axis limit to automatic whereas in the diagram at the right I intentionally limited the y-axis range from -40 to 0. Notice that the marker works fine for the case the y-axis i set to auto, but it is not possible to place a marker in the diagram at the right. I guess this stuff masks somehow the issue I reported above...

https://drive.google.com/open?id=1bFpisFFikWD49i8kEIEutG_a-H11_Zsh

[guitorri]

I could reproduce the crash with /qucs-test/testsuite/SP_Sparam_diagrams_prj, see video

Info from the console:

Assertion failed: (SplPosX->isPt()), function moveLeftRight, file marker.cpp, line 391.

Info from the crash report:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib          0x00007fffa5398d42 __pthread_kill + 10
1   libsystem_pthread.dylib         0x00007fffa5486457 pthread_kill + 90
2   libsystem_c.dylib               0x00007fffa52fe420 abort + 129
3   libsystem_c.dylib               0x00007fffa52c5893 __assert_rtn + 320
4   org.qucs                        0x000000010719e934 Marker::moveLeftRight(bool) + 660 (marker.cpp:397)
5   org.qucs                        0x0000000106f46005 Schematic::markerLeftRight(bool, Q3PtrList<Element>*) + 85 (schematic_element.cpp:942)
6   org.qucs                        0x0000000106f2fe69 QucsApp::slotCursorLeft(bool) + 137 (qucs_actions.cpp:1000)

Note that Markers can be added to the trace before the trace passes the y-axis clip. The trace segment just after the y-axis clip, markers are added at the origin. This is a bug. If the marker is moved across the y-axis clip region, the application crashes.

On qucs-0.018 if the marker is moved across the y-axis clip, the marker is placed temporarily at the origin, and the marker returns to the trace once points are inside the y-axis range.

[felix-salfelder]

On qucs-0.018 if the marker is moved across the y-axis clip, the marker is placed temporarily at the origin, and the marker returns to the trace once points are inside the y-axis range.

thanks, reproduced.

shouldn't the marker simply walk along the boundary of the visible area? that could use a project_to_visible_area(..)... back to workarounds:

i remember there were control tokens scattered across the dataset. i guess with these tokens in, the marker move code needs one extra "jump segment" case.

line 402. if (pointing to separator token) { ++pos; assert(is_pt()); }

a similar conditional is required in move_left and in findmarker_after_click(x,y)..

perhaps OT, but still: soft assertions might be more appropriate, until there is a way to enforce consistency. if(!is_pt()) {unreachable();}, with unreachable(); just reporting, not inducing a crash.

[andresmmera]

Hum, what about doing

while ((SplPosX != SplPosD->end()) && (!SplPosX->isPt()) )++SplPosX; for the movements to the right and while ((SplPosX != SplPosD->begin()) && (!SplPosX->isPt()) )--SplPosX; to the left in Marker::moveLeftRight()? I think that's simpler than the token system you suggest, isn't it?

I've quickly tried this and it seems to work. Perhaps this could be inefficient for large graphs... Screencast: https://drive.google.com/open?id=1CfoeDhltnbUbsQ-KkY3HrvdswWpIcPre

[felix-salfelder]

seems to work

great. i'm sorry i haven't come to try it.

I think that's simpler than the token system

i don't see that, because it is roughly what i had described. control-tokens are the ones that are "non-points".

except you used a while loop.. are there cases where multiple separators are in the data set? supporting this, will hide bugs in other places and probably backfire later.

i'd prefer a more explicit version that increments at most twice and includes some checking. how about

if(SplPosX == SplPosD->end()){
    // nothing to do.
}else if(!SplPosX->isPt()){
    // out of range, somethings wrong?!
    // warn? throw? assert?
} else {
    // we are in the middle of it
    ++SplPosX;
    if(SplPosX == SplPosD->end()){
        // nothing else to do.
    }else if(SplPosX->isPt()){
       /// good. lets use this.
    }else{
         ++SplPosX;
         // do we want separators just before end?
         assert(SplPosX == SplPosD->end() || SplPosX->isPt());
    }
}

maybe the boundaries need some checking. (as it is) move_right could move past the data, (how) is this handled in the other code?

[felix-salfelder]

did a rebase, and added the skip code. can't do any testing right now...

[andresmmera]

I'll try to do some testing here during the next few days...

[andresmmera]

Honestly, I haven't taken a look at the commit code yet... but I'll try to I noticed that the marker skips the trace portion that lies outside the diagram, but when doing that, the marker points at the origin. On the other hand, the marker doesn't skip the gap if it goes to the left. Screencast here.