-
Notifications
You must be signed in to change notification settings - Fork 61
average profiles now get slope feature as well where slope is in orange. #324
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
Conversation
|
Yes the new profile curve class knows how to plot the slope error. xDel is the x dimension over which the slope will be calculated. hDelLimit is how high the y can vary before the slope exceeds the limit set by the user. |
githubdoe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why you added the code to set the slope settings. I would think they were already set.
|
🚀 New build available for commit |
No I had to set the slope settings. Because just above is that "new" which created an instance of class ProfileCurve and so I had to set the boolean m_showSlopeError with setSlopeSettings(). The boolean isn't a static. It's a normal member variable. If you look at the code in entirety there is a section that creates the single profiles, the 16 profiles and the average profiles and each section creates their own instances and each section of code needs to set the slope flag thing. |
githubdoe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my mistake I was in a hurry and did not see what was really getting set.
| left.append(right); | ||
|
|
||
| QwtPlotCurve *cprofileavg = new QwtPlotCurve( name + " avg"); | ||
| ProfileCurve *cprofileavg = new ProfileCurve( name + " avg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a memory leak here.
A new with no delete and no Qt object with parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting point. I'll look at this today.
|
I'm going to merge this now and probably create a release (8.3.2) and if I agree there is a memory leak (quite likely but not 100% definite) I will create an "issue" and also a PR. Actually it's pretty easy I suppose to create a destructor and put a break point on it. When creating all new profiles the old ones need to get deleted. |
|
I think it's fine (no leaks). These 2 lines of code have a second default parameter that deletes everything. I think. So the items created are "attached" before plotting them. Then the next time you change what profiles you create these are called at the start: I'll create a destructor to be sure. But by default those detachItems() delete everything that was attached earlier. I hope. |
|
Confirmed! No memory leak. |
|
During review I though this was a new variable so I did miss the existing attach. |
I don't understand what I did but it seems to work. I don't understand the calculations of xDel and hDelLimit and so on. Well I get lambda - that's the wavelength. But it seems to work perfectly so I think I did it right.
All the code is cut and pasted except as you can see I changed an object from class QwtPlotCurve to ProfileCurve (ProfileCurve has ability to show slope and QwtPlotCurve does not).
Anyway I tested it against the non average and it seems to show slope in the same steepness.