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

Orthographic #1990

Merged
merged 3 commits into from May 31, 2016

Conversation

Projects
None yet
3 participants
@danlipsa
Contributor

danlipsa commented May 24, 2016

No description provided.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 24, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 24, 2016

@doutriaux1 @aashish24 Please review. Note that labels are wrong for mercator and orthographic projections as proj4 returns infinity for certain combination of values. We can fix the labels in a different PR.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

@danlipsa this is great. If is possible, can we fix the legend as well in the same branch since in the new baselines legend look more wrong now.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 26, 2016

@aashish24 @doutriaux1 Looking at the code, I would fix the tics/labels in a more general way, fixing it in a simple/specific way is not easy to do. The problem is that the code assumes that the outline of the data in lon/lat space results in an outline in projected space. This is not true for mercator and orthographic projections. I want to fix this by getting the proper outline - where data is not projected to infinity and passing that to the vcs routine that computes the location of the ticks/labels. I think this would affect many baselines so I would do it in a separate PR.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 26, 2016

@aashish24 @doutriaux1 This also seems to be a big/disruptive change, so we have to decide if we want it for this release.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 26, 2016

@aashish24 @doutriaux1 I'll look some more. Maybe I can do something simple for mercator. Oblique orthographic is a tough case, as the outline is not on a latitude or longitude.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 26, 2016

For future reference:
an oblique orthographic projection:
test_vcs_boxfill_orthographic
the points hidden from the lon / lat data
hidden

@danlipsa danlipsa force-pushed the orthographic branch from 8cae1c2 to 965eb69 May 26, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 26, 2016

Maybe I can do something simple for mercator.

Sounds good to me.

We can handle oblique one in the next PR.

@danlipsa danlipsa force-pushed the orthographic branch from 965eb69 to 83b9efb May 27, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 27, 2016

@aashish24 @doutriaux1 I fixed the labels for mercator and the labels for orthographic look a lot better. There are also some aeqd labels that were fixed in the process. Please review - you'll have to download the baselines to see all of them: 49 changed 😄

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 27, 2016

@danlipsa ubuntu results at: https://open.cdash.org/viewTest.php?onlyfailed&buildid=4385602

The following tests FAILED:
     49 - test_vcs_boxfill_orthographic_45 (Failed)
     50 - test_vcs_boxfill_orthographic_90 (Failed)
     87 - test_vcs_background_mode_rotate (Failed)
    286 - test_vcs_basic_isofill_masked_aeqd_proj (Failed)
    362 - test_vcs_isoline_width_stipple (Failed)
    460 - test_vcs_line_patterns (Failed)

@danlipsa danlipsa force-pushed the orthographic branch from 83b9efb to 6d8eba8 May 27, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 31, 2016

49 - test_vcs_boxfill_orthographic_45 (Failed)
50 - test_vcs_boxfill_orthographic_90 (Failed)

I added the missing test file

87 - test_vcs_background_mode_rotate (Failed)

seg fault - not sure what this is.

286 - test_vcs_basic_isofill_masked_aeqd_proj (Failed)

this seems to have the old baseline.

362 - test_vcs_isoline_width_stipple (Failed)
460 - test_vcs_line_patterns (Failed)

the stipples look like they are shifted. However on both my linux and my mac the are like in the baselines. Should we add an alternate baseline? I think that would be ok as long as this shifting is deterministic and we have only two versions.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 31, 2016

@doutriaux1 All tests pass on both my mac and my linux box.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 31, 2016

@danlipsa I updated the branch, but I'm not sure anything is going to pass any longer since @aashish24 merged his vector branch in. Do you mind double checking the baselines? I'm rebuilding this from scratch on my Ubuntu.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 31, 2016

@doutriaux1 I will rebase and push a new version.

danlipsa added some commits May 23, 2016

BUG #1985: orthographic projection plot is empty
This is because proj4 sets points that are not visisble to infinity.
We set those points to 0 and hide them.
Remove code that results in non-deterministic behavior for test_vcs_b…
…oxfill_mercator

We deal with points set to infinity by setting them to 0 and hidding them.
See the following commit: BUG: orthographic projection plot is empty

@danlipsa danlipsa force-pushed the orthographic branch from c493624 to e489972 May 31, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 31, 2016

@danlipsa see:   https://open.cdash.org/viewTest.php?onlydelta&buildid=4390366
It was probably ran before you get your updated baselines in. Let me know when you want me to re-run ctest.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 31, 2016

@danlipsa ok now I get: https://open.cdash.org/viewTest.php?onlyfailed&buildid=4390515

I don't think any failure is related to your branch any more, correct?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 31, 2016

@doutriaux1 That is right. I just merged a fix for test_vcs_png_window_resize. This should be ready to merge. Make sure you merge the data as well.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 31, 2016

Or I can merge if you prefer.

@doutriaux1 doutriaux1 merged commit 486375a into master May 31, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@doutriaux1 doutriaux1 deleted the orthographic branch May 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment