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

Grib backport #111

Merged
merged 5 commits into from
Feb 14, 2019
Merged

Grib backport #111

merged 5 commits into from
Feb 14, 2019

Conversation

did-g
Copy link
Contributor

@did-g did-g commented Dec 26, 2017

Hi,

  • backport unrelated stuff from grib plugin record so grib and weather_routing source code don't diverge to much.
  • use new grib class format once, or if, integrated in opencpn.
  • use smart pointers and a weakly referenced grib record table for sharing grib record between configurations, if it's committed, it needs some cleanup, this commit trying to change as little as possible code.

I'm hijacking plugin API version for testing grib record format change...

For testing on linux adding plugins as subtree is the easiest. My default branch https://github.com/did-g/OpenCPN.git have a bunch of subtree plugins , may have too much unrelated WIP changes though.

Regards
Didier

@rgleason
Copy link
Contributor

Did-g, can I make a pr and merge, and it will work? It sounds like Sean needs to do some additional programming, and we will need a new api115. No problem there, but doesn't OpenCPN Alpha needs to have it too?

@did-g
Copy link
Contributor Author

did-g commented Dec 27, 2017

Hi,
API version 115 is already in Dave's master branch but you need both ocpn and weather routing PR pull/merge .

Unless you are trying to compute route with big grib or very small time steps (ie many interpolated records) this PR have no visible change.

I wouldn't spend time on it.

Regards
Didier

@rgleason
Copy link
Contributor

Thanks, I am using th Alpha version now, so I guess I'll merge it next time, or when Sean does.

@seandepagnier
Copy link
Owner

Are there any measured performance improvements from shared pointers here?

Is this safe to merge... will it be compatible with older grib plugins, or do we need to check the grib plugin version?

@did-g
Copy link
Contributor Author

did-g commented Dec 30, 2017

Hi,
There's a small speed improvement but the main advantage is that on a 32 bits system for big gribs without sharing records weather_routing reach 4 GB limit a lot earlier.
By using less memory there's another improvement even on 64 bits system:
Currently opencpn purges its charts cache if the whole process memory is closed to a 1 GB limit.
Even without very big grib or a lot of computed routes it can really slowdown opencpn to a point where restarting opencpn is the only option left.

This version isn't compatible with 4.8.0 there's no need for testing because API version 115 is not release yet and this version will fail to load on 4.8.0 or older.

If you want to release a new weather_routing version for released O version don't merge this PR.

@seandepagnier
Copy link
Owner

seandepagnier commented Dec 30, 2017 via email

@did-g
Copy link
Contributor Author

did-g commented Dec 30, 2017

My grib source? The best, me :)

I'm using Meteo France for hourly wind 0.01° and IFREMER for 15 mn current 0.0023° .
for example:
http://195.154.231.142/IFREMER/MANW250.grb
it's a 60MB for a 4x2 degrees 15 hours...

I'm having fun trying to find if it's worth it.

@seandepagnier
Copy link
Owner

There was no release, so I am ready to merge this, if you can resolve the conflicts.

@did-g
Copy link
Contributor Author

did-g commented Feb 1, 2018

Hi,
This PR is not compatible with current OpenCPN record format.

I'm pushing a second change with only gribrecord modifications since previous copy and paste gribrecord code in weather routing, if you commit this PR I will update this one (still trying to find a way to stay compatible with current OpenCPN).

Regards
Didier

@seandepagnier
Copy link
Owner

I will merge if you can resolve conflicts.

@seandepagnier
Copy link
Owner

ok. waiting until opencpn (in git master) is ready.

@rgleason
Copy link
Contributor

rgleason commented Feb 9, 2018

What was this PR #132 Grib BackportV2 Grib record changes from Opencpn for?
078c4d1

Should this one be closed because it is old?

@did-g
Copy link
Contributor Author

did-g commented Feb 11, 2018 via email

@rgleason
Copy link
Contributor

rgleason commented Mar 22, 2018

On Feb 8 Sean merged #133 Error in Boundary which keeps compatibility with 4.8.2 and earlier.

On Dec 30,2017 Did-g wrote:

This version isn't compatible with 4.8.0 there's no need for testing because API version 115 is not release yet and this version will fail to load on 4.8.0 or older.
If you want to release a new weather_routing version for released O version don't merge this PR.

So I believe that Grib Backport #111 this should not be merged until we move on to wxWidgets 3.1.1 and working at the head on API 16 and v4.99.

@seandepagnier
Copy link
Owner

Is this ready to merge or should we wait for O5 release?

@did-g
Copy link
Contributor Author

did-g commented Feb 7, 2019 via email

@did-g
Copy link
Contributor Author

did-g commented Feb 14, 2019

Hi,
Should have right dependencies for Windows and OSX CI build.

@seandepagnier seandepagnier merged commit 4358838 into seandepagnier:master Feb 14, 2019
@did-g did-g deleted the grib_backport branch February 15, 2019 00:32
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.

3 participants