Terrain Loader multiple tesselector options

[laijackylai]

Recently I bumped into an issue when trying to create a simple terrain layer

Turns out the issue was due to the current tesselector that deckgl's terrain layer is using, which is Martini which only supports square images as an input (kindly pointed out by @ibgreen)

Suggestion:

• Add Delatin or other tesselectors as an option in the terrain loader for user to choose the desired tesselector
• update the docs on deck.gl's terrain layer

[ibgreen]

@laijackylai Sounds like you were going to make a stab at this in your project. Do you want to make a PR here in loaders?

[kylebarron]

@laijackylai you'd probably want

1. a new loader option defined here, defaulting to Martini for backwards compatibility. https://github.com/visgl/loaders.gl/blob/141f7f7078b69b0643a9ff44a7cb2f91105baf1b/modules/terrain/src/terrain-loader.js#L18-L22

2. A switch here between martini and delatin that uses the loader option, but switches to Delatin when the images is not a power of two. https://github.com/visgl/loaders.gl/blob/141f7f7078b69b0643a9ff44a7cb2f91105baf1b/modules/terrain/src/lib/parse-terrain.js#L97-L102

3. Note that you'd want to do a little refactoring. Pretty much the only lines relevant specifically to Martini in getMartiniTileMesh are these three: https://github.com/visgl/loaders.gl/blob/141f7f7078b69b0643a9ff44a7cb2f91105baf1b/modules/terrain/src/lib/parse-terrain.js#L76-L78

You'd basically just replace those three lines of Martini with

const tin = new Delatin(terrain, width, height);
tin.run(meshMaxError);
const {coords, triangles} = tin; // get vertices and triangles of the mesh
const vertices = coords;

and you might not need to change anything else in the rest of the code.

[laijackylai]

@ibgreen I'll create a new test proj and copy the relevant files there for testing, and when it's done I'll let you guys have a look. Thank you

@kylebarron Thank you for your advice, I have gone through the code and yeah, those are the places I will be changing. Also terrain layer in deck.gl also needs a small update to let users parse in an additional parameter to select their desired tesselector.

I'll update you guys and let you guys test it once it's done. Thanks again

[kylebarron]

Also terrain layer in deck.gl also needs a small update to let users parse in an additional parameter to select their desired tesselector.

That should be handled through the loadOptions prop, but I'd have to check it's passed through correctly

[laijackylai]

That should be handled through the loadOptions prop, but I'd have to check it's passed through correctly

got it thank you

[laijackylai]

https://github.com/laijackylai/loadersgl-tesselector

Here is the basic example I've made, delatin is running ok but @kylebarron I dont know why there're artifacts with the mesh generated by delatin.

Also, I don't know whether it's my code or martini is a lot faster in terms of performance

[kylebarron]

martini is expected to be 2-3x faster or so.

I haven't used Delatin in JS, just in Python, so it's hard for me to say if something is wrong, especially because it's hard to see a diff in a separate repo

[laijackylai]

let me try digging a bit deeper

[laijackylai]

I found the reason, while constructing the mesh, width + 1 and height + 1 need to be passed instead of actual width and height of the image. It's working now

[laijackylai]

@ibgreen @kylebarron please take a look at the code, see if it's ok. Then we can make a PR? I only changed 2 files in loaders.gl and 1 file in deck.gl

[kylebarron]

Would it be possible to make a PR first? It's hard to see what changed otherwise.

[laijackylai]

got it

[laijackylai]

https://github.com/visgl/loaders.gl/pull/1372

[ibgreen]

@laijackylai Thanks for implementing this. Would be good to add an update to the whats-new section as well (separate PR) if you have time

https://github.com/visgl/loaders.gl/blob/master/docs/whats-new.md

[laijackylai]

Sure let me give it a go

[laijackylai]

@ibgreen should this page be updated as well? https://github.com/visgl/loaders.gl/tree/master/modules/terrain

[ibgreen]

Yes, good catch, this file doesn't follow the style of most top-level READMEs. Those are just used for npm publishing. We normally don't provide details there but link to the reference the documentation, where we have attribution. We can do that in separate PR.