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

[3D] 3d tilt #2609

Merged
merged 2 commits into from Sep 2, 2015
Merged

[3D] 3d tilt #2609

merged 2 commits into from Sep 2, 2015

Conversation

fredj
Copy link
Member

@fredj fredj commented Aug 27, 2015

No description provided.

@fredj fredj force-pushed the 3d_tilt branch 7 times, most recently from c6e9c65 to f6dd9d6 Compare August 27, 2015 12:12
@fredj
Copy link
Member Author

fredj commented Aug 27, 2015

@fredj
Copy link
Member Author

fredj commented Aug 31, 2015

demo updated

@fredj
Copy link
Member Author

fredj commented Sep 1, 2015

Thanks for any review.

I don't really like the new disable3d property for the swissimage layer. Any other proposal welcome !

@fredj fredj changed the title [WIP] 3d tilt [3D] 3d tilt Sep 1, 2015
// true is the selected background layer is not 3d compatible.
scope.disabled = false;
scope.$on('gaBgChange', function(evt, value) {
scope.disabled = value.id == 'ch.swisstopo.swissimage';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use value.disable3d here?

Copy link
Member Author

Choose a reason for hiding this comment

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

disable3d property is only available in the background selector directive, not here

Copy link
Contributor

Choose a reason for hiding this comment

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

it shouldn't, The valueparam should contains the disable3dproperty.

valueis a bg object with an id, a label and a disable3dif it exists

Copy link
Member Author

Choose a reason for hiding this comment

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

oups, I was searching in the evt object ...

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, code updated

@elemoine
Copy link
Contributor

elemoine commented Sep 1, 2015

Question for everyone: shouldn't the mouse cursor change when hovering the 3D button and when 3D is disabled because the background layer is swissimage? Currently, the cursor is still a hand, which I think is confusing.

@elemoine
Copy link
Contributor

elemoine commented Sep 1, 2015

@fredj, when I switch to 3D I sometimes get some big jump with a blue background before actually seeing the expected background. Is this a known issue?

@elemoine
Copy link
Contributor

elemoine commented Sep 1, 2015

@fredj, when I switch to 3D I sometimes get some big jump with a blue background before actually seeing the expected background. Is this a known issue?

And when this occurs the map is not tilted after switching to 3D. I am not even sure the location is the same as the previous, 2D, location.

@fredj
Copy link
Member Author

fredj commented Sep 1, 2015

@fredj, when I switch to 3D I sometimes get some big jump with a blue background before actually seeing the expected background. Is this a known issue?

yes, it's a known ol3-cesium issue

@fredj
Copy link
Member Author

fredj commented Sep 1, 2015

Question for everyone: shouldn't the mouse cursor change when hovering the 3D button and when 3D is disabled because the background layer is swissimage? Currently, the cursor is still a hand, which I think is confusing.

This is correct. The UI will by done by swisstopo.

@fredj
Copy link
Member Author

fredj commented Sep 1, 2015

I've fixed the css cursor when the feature is disabled or not available

// true is the selected background layer is not 3d compatible.
scope.disabled = false;
scope.$on('gaBgChange', function(evt, value) {
scope.disabled = !!value.disable3d;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why !!? value.disable3d is a boolean value already, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

boolean or undefned

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes, true.

@elemoine
Copy link
Contributor

elemoine commented Sep 1, 2015

No more comments from me.

@gjn
Copy link
Contributor

gjn commented Sep 2, 2015

LGTM. Artwork still remains to be done. But I'll merge this.

Thanks alot.

gjn added a commit that referenced this pull request Sep 2, 2015
@gjn gjn merged commit 6a4e715 into master Sep 2, 2015
@gjn gjn deleted the 3d_tilt branch September 2, 2015 07:04
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

4 participants