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

Heat Map at Surface #2027

Merged
merged 4 commits into from Mar 29, 2019
Merged

Heat Map at Surface #2027

merged 4 commits into from Mar 29, 2019

Conversation

atdotde
Copy link
Collaborator

@atdotde atdotde commented Mar 25, 2019

Describe the pull request:

  • Bug fix
  • Functional change
  • New feature
  • Code cleanup
  • Build system change
  • Documentation change
  • Language translation

Pull request long description:

Part of the decompression is done at the surface while breathing air again. This PR adds the possibility to show this in the planner by extending the dive by a final segment at zero depth breathing air. This way, tissue saturations and in particular the heat map becomes visible after reaching the surface.

Changes made:

Related issues:

Additional information:

Release note:

Documentation change:

Mentions:

Copy link
Collaborator

@dirkhh dirkhh left a comment

Choose a reason for hiding this comment

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

Typo in the last commit message suface vs surface.
My review is more nit-picky than usual, my apologies. I wish I'd always do such thorough reviews - this just happens to be something that was easy to understand for me, so I felt ambitious :-)

core/planner.c Show resolved Hide resolved
core/planner.c Outdated
current_cylinder = i;
break;
}
plan_add_segment(diveplan, 600, 0, current_cylinder, 0, false, OC);
Copy link
Collaborator

Choose a reason for hiding this comment

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

adding this unconditional, instead of as an option. Hmm. Interesting. I didn't expect that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UI is added in second commit as you will see. This first commit is to get the logic right.

core/plannernotes.c Show resolved Hide resolved
if (result.isEmpty()) {
Q_ASSERT("can't find a spot in the dataModel");
hide();
return -666;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The number of the beast as an error number is cute... but I'd prefer something with a symbolic name that is then checked by the caller... because the caller above uses this as a depth (admittedly just to compare against a constant, but still... worst case return "-1" which you define as "ABOVE_SURFACE")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I needed some out of bounds special value to signal error (before, when this was part of the other function which is of type void, this was simply a return;.

-1 would not really work (yes, I just realised that the conditional below should take that into account) as that is a valid depth value of a somewhat noisy depth sensor. So I needed a more negative number. But yes, that should be a constant...

void DiveEventItem::recalculatePos(bool instant)
{
if (!vAxis || !hAxis || !internalEvent || !dataModel)
return;


Copy link
Collaborator

Choose a reason for hiding this comment

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

a second empty line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops

QModelIndexList result = dataModel->match(dataModel->index(0, DivePlotDataModel::TIME), Qt::DisplayRole, internalEvent->time.seconds);
if (result.isEmpty()) {
Q_ASSERT("can't find a spot in the dataModel");
hide();
return;
}
int depth = depthAtTime(internalEvent->time.seconds);
if (depth < 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

and then here you could compare with == to your symbolic constant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should definitely test against the constant (which should be negative enough not to be a possible value of a depth sensor)

core/planner.c Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -1,3 +1,4 @@
- Planner: Allow for a final dive segment at the surface
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm being picky, but is this a "dive" segment or is this a "deco" segment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do. Expect an update tonight (old world time).

to display the deco parameters at the surface,
in particular tissue saturation and heat map.

Suggeted-by: Matthias Heinrichs <info@heinrichsweikamp.com>
Signed-off-by: Robert C. Helling <helling@atdotde.de>
Signed-off-by: Robert C. Helling <helling@atdotde.de>
Signed-off-by: Robert C. Helling <helling@atdotde.de>
@atdotde
Copy link
Collaborator Author

atdotde commented Mar 27, 2019

Rebased on current master and addressed comments.

Copy link
Collaborator

@dirkhh dirkhh left a comment

Choose a reason for hiding this comment

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

Still that typo in the final commit message:
"Make suface air..."
"suface" isn't a word I know...

it creapt in through the gaschange events.

Signed-off-by: Robert C. Helling <helling@atdotde.de>
Copy link
Collaborator

@dirkhh dirkhh left a comment

Choose a reason for hiding this comment

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

LGTM

@dirkhh dirkhh merged commit 30746e5 into subsurface:master Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants