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

feat(layer): add full support to scaling for elevation layers #1174

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

zarov
Copy link
Contributor

@zarov zarov commented Jul 3, 2019

Changing the scale property of an ElevationLayer can changed the
aspect of the visual, allowing exageration of the elevation of the
globe, as in the tiff.html example.

This is an up-to-date version of #511

@zarov zarov added ready ✔️ feature 🍏 Adds a new feature labels Jul 3, 2019
@zarov zarov added this to the 2.13.0 milestone Jul 3, 2019
@gchoqueux gchoqueux mentioned this pull request Jul 3, 2019
Copy link
Contributor

@gchoqueux gchoqueux left a comment

Choose a reason for hiding this comment

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

Thanks, we will finally be able to integrate this functionality!
I have just on comment on the code and a bug on OBB visibility :

There are culling errors with obb tile, see the bbox culling in camera and horizon culling.

bbox

max = Math.max(max, val);
min = Math.min(min, val);
}
max = Math.max(max, val);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this clamp was to handle the elevation error (with huge negative elevation)

Copy link
Contributor Author

@zarov zarov Jul 10, 2019

Choose a reason for hiding this comment

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

I understand, but then the example won't work, as there is huge negative elevation, for example Marina trench at -11.000m, times the scale is way beyond the -10 authorized here.

Copy link
Contributor

Choose a reason for hiding this comment

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

you could add layer option to clamp the elevation?

@zarov zarov force-pushed the exagerated-geoid branch 2 times, most recently from 4c4b444 to 13e4d74 Compare July 19, 2019 06:48
@zarov zarov added wip 🚧 Still being worked on ready ✔️ and removed ready ✔️ wip 🚧 Still being worked on labels Jul 22, 2019
@zarov
Copy link
Contributor Author

zarov commented Jul 23, 2019

Up to date and ready, I couldn't get to perfect bbox culling, so I'm living like it is now, as it is only for special case that this is visible (with great exageration of altitude).

@@ -253,6 +253,12 @@ class LayeredMaterial extends THREE.RawShaderMaterial {
getElevationLayer() {
return this.layers.find(l => l.id === this.elevationLayerIds[0]);
}

setElevationScale(scale) {
if (this.elevationLayerIds.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you replace by if (this.elevationLayerIds.length)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@zarov zarov force-pushed the exagerated-geoid branch from 13e4d74 to 8a3d83f Compare July 29, 2019 12:48
@gchoqueux
Copy link
Contributor

I push a fix on @gchoqueux / fix_geoid. Could you comment and test?

@zarov
Copy link
Contributor Author

zarov commented Jul 29, 2019

It is better, thanks, but it is not perfect yet: I think it will be very difficult to have a perfect result here anyway.

@zarov zarov force-pushed the exagerated-geoid branch from 8a3d83f to 2b363b2 Compare July 30, 2019 12:01
@zarov
Copy link
Contributor Author

zarov commented Jul 30, 2019

So I added your suggestion, and rebase it, now it should be good to go !

@gchoqueux
Copy link
Contributor

could you open folder gui, to see directly the scale slider?

@zarov zarov force-pushed the exagerated-geoid branch from 2b363b2 to 36ffcd6 Compare July 30, 2019 13:19
@zarov
Copy link
Contributor Author

zarov commented Jul 30, 2019

Done

Changing the `scale` property of an `ElevationLayer` can changed the
aspect of the visual, allowing exageration of the elevation of the
globe, as in the tiff.html example.

This is an up-to-date version of iTowns#511
Copy link
Contributor

@gchoqueux gchoqueux left a comment

Choose a reason for hiding this comment

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

since we've been waiting for this feature: Thanks!

@zarov zarov merged commit e736d1f into iTowns:master Jul 30, 2019
@zarov zarov deleted the exagerated-geoid branch July 30, 2019 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🍏 Adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants