search

Qucs / qucs

Qucs Project official mirror
http://qucs.sourceforge.net/
GNU General Public License v2.0
1.14k stars 208 forks source link

3D cartesian plot is broken #386

Closed ra3xdh closed 7 years ago

ra3xdh commented 8 years ago

I have just found another critical bug in Qucs-GUI. 3D-cartesian plot is broken in master. Open two schamtics with 3D plots in 0.0.18 and 0.0.19 and compare results to reproduce this bug. 3D plot in 0.0.19 looks unreadable. Here is example screenshot. Left is Qucs-0.0.18, right is Qucs-0.0.19. 3d-diagr

felix-salfelder commented 8 years ago

it seems, the new code does not print as many cross connections.

the missing blue sample dots in the left picture are more interesting. is this related to the z buffer? what happens if you slightly turn the plot to the left (= move the camera to the right), will they reappear?

please post the data file and the schematic somewhere, i will have a look.

ra3xdh commented 8 years ago

Here is test schematic https://gist.github.com/ra3xdh/eb92c81d7d72d401668b/raw/12be21db82ff579e91a9a7e4aa63370a25df2ba3/diode_dblswp_qucs.sch 3D cartesian plot is embedded in schematic.

what happens if you slightly turn the plot to the left (= move the camera to the right), will they reappear?

Yes, missing points could appear when plot is rotated at specific angle around Z-axis.

felix-salfelder commented 8 years ago

thanks

whether the cross connections are painted or not depends on the angle. must be a bug in the zbuffer code. also, i get a lot of "dangerous: returning negative screen coordinate" messages, which may or may not be related...

felix-salfelder commented 8 years ago

apparently, hideLines is broken for 3dplots. i am working on this in #370. previously the z buffer has been monkey-patched into the 2d ploting routines. these have been replaced by the QtPainter.

i think i will implement 3dgraphs with their own drawings...

guitorri commented 8 years ago

If at a certain angle the user still can get a useful image, then I will count it as know issue/regression. I will move this to the next release, I hope it is ok if you guys.

felix-salfelder commented 8 years ago

i have failed to make the old code work again. wasted a lot of time on this :(

using qt for the 2d plots is working pretty well. i think we should head for something like qwtplot to do the 3d plots. unfortunately qwtplot does not support multiple layers (yet)...

ldpgh commented 7 years ago

Simple check with x=5 SweepPts : y=5 SweepPts

Screenshots of the different revisions ... looks like an evolution

ldpgh commented 7 years ago

Simple check with x= 3 ... 5 SweepPts : y=5 SweepPts

Screenshots using 0.0.19 and different point size of the x-Sweep

ldpgh commented 7 years ago

Is there any Unit-test associated to rect3ddiagram.cpp?

felix-salfelder commented 7 years ago

i remember we had been discussing unit tests for the GUI, something about graphical diffs and they are somewhere on a whishlist. @ldpgh do you know how to implement that? please don't hesitate!

ldpgh commented 7 years ago

Environment: Win7, qucs-HEAD, CMake in QtCreator 3.1.2 @ Qt4.8.6, mingw482.

There is a significant difference of appearance between debug-mode (significant more line segments visible in 0.0.19, regardsless breakpoints) vs. non-debug-mode.

Have you experienced similar behavior?

felix-salfelder commented 7 years ago

as you might have guessed, the debug mode must not interfere with the behaviour of the programme (except for printout to stderr), so that's another bug.

in3otd commented 7 years ago

...interesting, maybe some floating point comparisons are giving different results, since the variables may end in registers instead of memory. I think we saw something similar in the simulator in the past.

Doing a quick diff of rect3ddiagram.cpp between version 0.0.6 and the current one doesn't appear to show any big changes (besides some slightly different rounding/truncation), at a first glance, but I may well have overlooked something.

ldpgh commented 7 years ago

@in3otd ... current version vs version 0.0.6 ... Do you mean Qucs version 0.0.6, because for rect3ddiagram.cppit looks to me there are 2 major implementation changes:

In addition it seems some changes are caused by graph.cpp/.h respective lead to changes in graph.h/.cpp. It's not expect with rect3ddiagram beeing the only 3D diagram and the associated requirements.

ldpgh commented 7 years ago

Not verified/debugged yet . . . just guessing (!!!) . . . will check it next days. The differences smells like "malloc without memset" (e.g. removeHiddenLines()). It would explain some strange behavior related how the application is started respective the previous run.

What is the more appropriate direction . . . taking into accound all the side effects

  1. fix current revision with all the floats
  2. lift up the implementation of 0.0.9 (which sounds gracy) to match current 0.0.19 implementation of graph.h/.cpp and diagram.h/.cpp
ldpgh commented 7 years ago

Finding-1

(1) Start in Run-Mode (Ctrl-R) ...

(2) Load rect3ddiagram1x_test_sch.sch qucs_rect3ddiagram_0 0 19__one_kind

(3.1) Load rect3ddiagram7x_test_sch.sch (3.2) Close Load rect3ddiagram7x_test_sch.sch (3.3) Close Load rect3ddiagram1x_test_sch.sch

(4) Load rect3ddiagram1x_test_sch.sch qucs_rect3ddiagram_0 0 19__other_kind

Note-1: Closing order @ 3.2/3.3 has impact : 7x before 1x Note-2: Hide/Unhide of SelectionWindow has impact too !

ldpgh commented 7 years ago

Finding-2

with hiddenenabled and return value of isHidden() overriden to true all drawn line segments look OK ... means looks the same as hidden=false. This is surprising to me, because I have not just missing segments. Sometimes line segmets start/end outside the diagram-area, which would point to the framebuffer and associated files. qucs_rect3ddiagram_0 0 19__outlier

felix-salfelder commented 7 years ago

... also try to plot 3D data (i.e. sweep over 3 parameters). that gives a rough idea of what the code is trying to do. then 4.

ldpgh commented 7 years ago

It's cool stuff (with hideLine=true) ... for this reason it make sense to spend some time with it. qucs_rect3ddiagram_0 0 9__3waves

And a snapshot with 4 sweep for completeness. qucs_rect3ddiagram_0 0 9__3waves_5sweeps

And a chart with all line segments visible for comparison. qucs_rect3ddiagram_0 0 9__3waves_nohide

3 pictures added just to see the goal :-)

And a screenshot of the current status (0.0.19 @ Oct 2016) ... qucs_rect3ddiagram_0 0 19__3waves

ldpgh commented 7 years ago

I really like to get a feedback regarding appropriate direction . . . taking into accound all the side effects

  1. fix current revision 0.0.19 with all the floats.
  2. lift up again the implementation of 0.0.9 (which is the other side of the medal) to match current 0.0.19 implementation of graph.h/.cpp and diagram.h/.cpp. It's unknow to me what get lost in this case.

It guides my debug strategy/procedure ... even the recommended one may not be the best solution. Tried to handle both ... but keep going with both would require at least one more head on my shoulders ;-)

felix-salfelder commented 7 years ago

the reason why i rewrote parts of the plotting (mostly unmerged work) is, that i would like to read data from other sources than just .dat files. think of hdf5 or a simulator as a possible alternative souce (for now). the dat format is absolutely unsuited for this type of task, but unfortunately, the plotting routines more or less work on a binary image that is layed out like these .dat files... which is where the shit is hitting the fan.

that said, when i tried to fix the 3d plot (yes, i'd better be able to, but i wasnt), it was pretty clear to me that this would the wrong track. it never worked right (and not only the comments indicate that it is trial-and-error-spaghetticode). one part of this desaster is, that the 3dplot and the 2dplot are heavily entangled, and they should not be. if you can untangle it, then yes, lift 0.0.9. but please only for the 3d plotting. if you prefer a sane path, please help with #370, it fixes some other problems with the markers as well...

please let me know, if you need more info. will have more time for this after the symbols are in place...

ldpgh commented 7 years ago

... continuing debug with HEAD (9d924caa3879fbca978987a9fe964eadbc5f0fc4)

Finding-3 rect3ddiagram.cpp : isHidden() + hideLines=true (!)

rect3ddiagram.cpp : removeHiddenLines() + hideLines=true (!)

rect3ddiagram.cpp : calcLine(...)

ldpgh commented 7 years ago

Finding-4 rect3ddiagram.cpp : calcCoordinate3D(...) + hideLines=true (!)

p->x  = int(calcX_2D(x, y, zr) + 0.5 + xorig);
p->y  = int(calcY_2D(x, y, zr) + 0.5 + yorig);

. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . ^^^ Remove of 0.5 brings a couple of line segments back, but it seems to be just a sideeffect. Reopen of the schematic brings sometimes the old status back.

ldpgh commented 7 years ago

Modifications just in rect3ddiagram.cpp improve the behavior ... but not fixed to 100%.

Screenshot of the 0.0.19Mod @ 13th Nov 2016 qucs_rect3ddiagram_0 0 19mod13nov__3waves

Screenshot of the 0.0.19 @ Oct 2016 qucs_rect3ddiagram_0 0 19__3waves

In addition there are a test schematics ... maybe qucs-test is a good place for it as long as rect3ddiagram is part of the game.

Would like to have someone else to check the modification ... who spends a little bit of time?

ldpgh commented 7 years ago

Finding-5 rect3ddiagram.cpp : removeHiddenLines(...) . . . segmentation fault

In order to chase the random behavior the initialmalloc() for zMem& Memhas been changed from malloc( (Size+2)*sizeof(...)) -> malloc( 8*(Size+2)*sizeof(...)) ... just to prevent the realloc(). It seems as result of the extended allocated memory no more lines leaving the diagram area ... but it's hard to verify this assumption.

Adding test-case diode_dblswp_qucs.sch with 200pts @ SW1 has brought up a memory issue (segmentation fault at fprintf for debugging respective @ calcLine : pMem->x = ...) while turning the malloc back to the original settings (remove of 8*).

With the modified setting (8*(...)) diode_dblswp_qucs.sch runs with 20000pts @ SW1 and 50pts @ SW2 with no issues.

ldpgh commented 7 years ago

0.0.19Mod @ 13th Nov 2016 ... modifications are in rect3ddiagram.cpp . The file is based on commit https://github.com/Qucs/qucs/commit/9d924caa3879fbca978987a9fe964eadbc5f0fc4

Comparison ... 0.0.9 ... 0.0.19 ... 0.0.19 Mod @ 13th Nov 2016 ... and how it looks at my screen. cmp_revs_small_000 cmp_revs_small_test cmp_revs_test_sch cmp_revs_7x cmp_revs_dio cmp_revs_waves3x_sweeps4x cmp_revs_waves3x_test

felix-salfelder commented 7 years ago

wow. care to open a PR with the modified recd3dthing.cpp?

guitorri commented 7 years ago

@ldpgh can you share your fix? If you do we can include it already on 0.0.19 which is about to be released.

ldpgh commented 7 years ago

@ guitorri & Felix ... "share the files" is the plan.

Understand Git/Github PR-procedure is still a hurdle. And I'm still fiddling with my debug comments spread over the entire file ... to a large degree no longer valid.

Is there any deadline? My plan is to get it done until end of this week.

guitorri commented 7 years ago

I tentatively put December 1st for the release. So take your time and let us know if you need any assistance. This might help: https://github.com/Qucs/qucs/wiki/Contribution. In particular the workflow section.

felix-salfelder commented 7 years ago

debug comments spread over the entire file

git add -p might be useful. rescued me a couple of times (out of similar situations).

guitorri commented 7 years ago

Fix merged into release-0.0.19. Thank you ldpgh.