rivetTDA / rivet

RIVET is a tool for Topological Data Analysis, in particular two-parameter persistent homology.
GNU General Public License v3.0
73 stars 24 forks source link

enhanced gui functionality #108

Closed SimonSegert closed 6 years ago

SimonSegert commented 6 years ago

Added:

Changed:

mlwright84 commented 6 years ago

Simon, thanks for your work on this! It looks very good overall.

However, starting with commit 5c4c3db, the viewer crashes (with a segmentation fault) as soon as it appears on the screen. The cause seems to be the following delete statements in SliceDiagram::redraw_labels()

delete rect1;
delete rect2;
...
delete rect6;

It seems that rect1, ..., rect6 are null pointers when they are deleted, and this causes a crash (on some systems, but evidently not on yours or Mike's). Could we edit the code to remove these delete statements? For example, could we simply move the rectangles without deleting them?

SimonSegert commented 6 years ago

Hi Matthew,

Thanks for your feedback!

I added a new commit, and I’m interested to know whether it fixes the issue on your machine (I just “new”-ed all the rectangles before the function gets called). I agree that deleting and new-ing the rectangles every time seems a little wasteful; I wasn’t sure how to move them without using a QPainter; do you know of a more proper way to move them?

Thanks!

On Feb 27, 2018, at 5:06 AM, Matthew Wright notifications@github.com wrote:

and this causes a crash (on some systems, but evidently not on yours or Mike's). Could we edit the code to remove these delete statements? For example, could we simply m

mlwright84 commented 6 years ago

Simon, your change fixed the issue on my machine. However, I think that a more efficient solution would avoid the new and delete commands in redraw_labels(). This should be possible by using functions such as:

Could we try this approach?

SimonSegert commented 6 years ago

Thanks Matthew, I’ll try that.

On Feb 27, 2018, at 2:39 PM, Matthew Wright notifications@github.com wrote:

Simon, your change fixed the issue on my machine. However, I think that a more efficient solution would avoid the new and delete commands in redraw_labels(). This should be possible by using functions such as:

Resize rectangles with QGraphicsRectItem::setRect() Move rectangles/text with QGraphicsItem::SetPos() Ensure that rectangles/text stay on top of the other visualization elements with QGraphicsItem::setZValue() Could we try this approach?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rivetTDA/rivet/pull/108#issuecomment-369000414, or mute the thread https://github.com/notifications/unsubscribe-auth/ATXUE0Eh0-8u6ryOvD6d82_9iTIiHmfiks5tZFnmgaJpZM4R5khE.

SimonSegert commented 6 years ago

Matthew, I just pushed a new version which avoids the deletes.

On Feb 27, 2018, at 2:39 PM, Matthew Wright notifications@github.com wrote:

Simon, your change fixed the issue on my machine. However, I think that a more efficient solution would avoid the new and delete commands in redraw_labels(). This should be possible by using functions such as:

Resize rectangles with QGraphicsRectItem::setRect() Move rectangles/text with QGraphicsItem::SetPos() Ensure that rectangles/text stay on top of the other visualization elements with QGraphicsItem::setZValue() Could we try this approach?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rivetTDA/rivet/pull/108#issuecomment-369000414, or mute the thread https://github.com/notifications/unsubscribe-auth/ATXUE0Eh0-8u6ryOvD6d82_9iTIiHmfiks5tZFnmgaJpZM4R5khE.

xoltar commented 6 years ago

Hi @SimonSegert could you please rebase this onto current master to address the conflicts & then I'll review? Sorry for the delay, I didn't get any notification from github that Matthew had requested me to review it.

Thanks!

SimonSegert commented 6 years ago

Hi @xoltar this pull request is no longer up to date. I will close it, and soon will open up another request on another branch. Sorry for the confusion!