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

Updated strategy for removing cylinders #885

Merged
merged 1 commit into from Feb 27, 2018

Conversation

sfuchs79
Copy link
Contributor

Describe the pull request:

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

Pull request long description:

Change the strategy when to allow cylinder removal from a dive:

  • Not remove when cylinder has gas switch events, in any other cases
    allow removal
  • Remove this whole "cylinder with same gas" thing being a criteria
    for cylinder removal

When removing a cylinder which has corresponding pressure info in
samples, also remove this pressure info from the samples.

This from my point of view simplifies our strategy for removing cylinders but also does some major changes:

  • This whole "it's ok to remove a cylinder if there is another cylinder with same gas" is removed.
  • Main remaining aspect for the decision if a cylinder can be removed outside the planner is "does it have gas change events" --> First cylinder will always have gas change event so never can be removed.

Changes made:

Related issues:

Closes #869

Additional information:

Release note:

Documentation change:

Mentions:

@sfuchs79 sfuchs79 changed the title RFC: Updated strategy for removing cylinders [RFC][WIP] Updated strategy for removing cylinders Nov 29, 2017
for (int i = 0; i < rowCount(); i++) {
if (mapping[divepoints[i].cylinderid] >= 0)
divepoints[i].cylinderid = mapping[divepoints[i].cylinderid];
}
Copy link
Member

Choose a reason for hiding this comment

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

would this } close the for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, indentation wrong. Will fix this... Other findings?

Copy link
Member

Choose a reason for hiding this comment

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

nope, can't seem to find anything else.

@sfuchs79
Copy link
Contributor Author

Ok, indentation thing fixed. Thanks for the review!
For this cylinder removal change we clearly need to wait to hear @dirkhh s opinion. Maybe also talk to Linus as well because this whole "same gas" strategy may be s.th. he once wanted to have.

@sfuchs79 sfuchs79 changed the title [RFC][WIP] Updated strategy for removing cylinders [RFC] Updated strategy for removing cylinders Dec 30, 2017
@sfuchs79
Copy link
Contributor Author

Small update done: Removed unused variable "cyl".

Change the strategy when to allow cylinder removal from a dive:
- Not remove when cylinder has gas switch events, in any other cases
  allow removal
- Remove this whole "cylinder with same gas" thing being a criteria
  for cylinder removal

When removing a cylinder which has corresponding pressure info in
samples, also remove this pressure info from the samples.

Signed-off-by: Stefan Fuchs <sfuchs@gmx.de>
@dirkhh
Copy link
Collaborator

dirkhh commented Feb 24, 2018

Has @torvalds looked at this? You may have to ping him on email as he has turned off GitHub notifications...

@torvalds
Copy link
Collaborator

Looks ok to me.

@sfuchs79
Copy link
Contributor Author

@torvalds
Thanks for reviewing this!
@dirkhh
I think with the positive feedback from Linus we can use this. Worst case I will get some negative feedback from other users AFTER this is in master but I think we would be able to deal with this as well.

@sfuchs79 sfuchs79 changed the title [RFC] Updated strategy for removing cylinders Updated strategy for removing cylinders Feb 27, 2018
@janmulder janmulder merged commit 4cbf8b8 into subsurface:master Feb 27, 2018
@sfuchs79 sfuchs79 deleted the bugfix_cylinder_removal branch February 28, 2018 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to delete cylinders and their associated pressure data after Liquivision data import
5 participants