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

#8055 allow to load 3dtiles tileset via viewer parameters #9021

Merged
merged 7 commits into from
Mar 24, 2023

Conversation

allyoucanmap
Copy link
Contributor

@allyoucanmap allyoucanmap commented Mar 15, 2023

Description

This PR introduces the possibility to include a 3D Tiles layer with the query parameters. The viewer switches to 3D view it it detect a 3D tiles layer among the one included in the query parameter action CATALOG:ADD_LAYERS_FROM_CATALOGS

Here the introduced structures:

The 3D tiles service endpoint does not contain a default property for the name of the layer and it returns only a single record for this reason the name used in the layers array will be used to apply the title to the added 3D Tiles layer:

{
    "type": "CATALOG:ADD_LAYERS_FROM_CATALOGS",
    "layers": ["My 3D Tiles Layer"],
    "sources": [{ "type":"3dtiles", "url":"https://example.com/tileset-pathname/tileset.json" }]
}

GET: #/viewer/config?actions=[{"type":"CATALOG:ADD_LAYERS_FROM_CATALOGS","layers":["My 3D Tiles Layer"],"sources":[{"type":"3dtiles","url":"https://example.com/tileset-pathname/tileset.json"}]}]

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Minor changes to existing features

Issue

What is the current behavior?

#8055

What is the new behavior?

It its possible to add 3D tiles layer with the query parameter action CATALOG:ADD_LAYERS_FROM_CATALOGS

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • No

Other useful information

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

I noticed that the 3D switch implementation starts from 2D, then commutes to 3D when the data is available.

This unnecessary rendering is a little ugly and may be confusing.
I noticed that instead if you put the proper parameters (used for 3D share for coordinates of the camera) this do not happen.

So I'd suggest to avoid this auto-switch to 3D option based on layer added, and ask explicitly parameters for 3D version of the map.


GET: `#/viewer/config?actions=[{"type":"CATALOG:ADD_LAYERS_FROM_CATALOGS","layers":["https://example.com/tileset-pathname/tileset.json"],"sources":[{"type":"3dtiles","url":"https://example.com/tileset-pathname/tileset.json"}]}]`

- Use the `title` property in the source object to assign the name to the layer
Copy link
Member

Choose a reason for hiding this comment

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

title is an identifier for the layers in 3D tiles? if so, please explain, something like

Suggested change
- Use the `title` property in the source object to assign the name to the layer
- Because `title` is the identifier used for 3D tiles. use`title` property in the source object to assign the name to the layer

Anyway, I should use name, and allow to use the title property for (maybe localized) items. This way it is conformant with the rest of the MapStoe model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the new configuration the title property is not visible anymore

web/client/epics/catalog.js Show resolved Hide resolved
@tdipisa tdipisa modified the milestones: 2023.02.00, 2023.01.01 Mar 21, 2023
@tdipisa tdipisa added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Mar 21, 2023
@allyoucanmap
Copy link
Contributor Author

I noticed that the 3D switch implementation starts from 2D, then commutes to 3D when the data is available.

This unnecessary rendering is a little ugly and may be confusing. I noticed that instead if you put the proper parameters (used for 3D share for coordinates of the camera) this do not happen.

So I'd suggest to avoid this auto-switch to 3D option based on layer added, and ask explicitly parameters for 3D version of the map.

moved the switch at higher level to ensure the map switch to 3D as soon as possible

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

LGTM, only one clarification comment

docs/developer-guide/map-query-parameters.md Show resolved Hide resolved
@landryb
Copy link
Collaborator

landryb commented Mar 24, 2023

Fwiw i've retested this PR after 64f6f0a and the switch to 3d mode is indeed done straight at viewer loading, much better. The layer title in the TOC is also correctly set from the layers parameter:

http://localhost:8081/#/viewer/config?actions=%5B%7B%22type%22:%22CATALOG:ADD_LAYERS_FROM_CATALOGS%22,%22layers%22:%5B%22My%20title%22%5D,%22sources%22:%5B%7B%22type%22:%223dtiles%22,%22url%22:%22http://3d.craig.fr/datasets/Aurillac/3dtiles/tileset.json%22%7D%5D%7D%5D

i've also tested adding two 3dtiles layer at once and it works, layer titles are properly set and match the order of entries in the sources & layers arrays:
http://localhost:8081/#/viewer/config?actions=[{"type":"CATALOG:ADD_LAYERS_FROM_CATALOGS","layers":["Aurillac","Domerat"],"sources":[{"type":"3dtiles","url":"http://3d.craig.fr/datasets/Aurillac/3dtiles/tileset.json"},{"type":"3dtiles","url":"https://3d.craig.fr/datasets/Domerat_bati3d/3dtiles/tileset.json"}]}]

And finally, adding two 3dtiles layers and a wms layer also works, the viewer is still directly loaded in cesium mode from the start:
http://localhost:8081/#/viewer/config?actions=[{"type":"CATALOG:ADD_LAYERS_FROM_CATALOGS","layers":["Aurillac","Domerat","commune"],"sources":[{"type":"3dtiles","url":"http://3d.craig.fr/datasets/Aurillac/3dtiles/tileset.json"},{"type":"3dtiles","url":"https://3d.craig.fr/datasets/Domerat_bati3d/3dtiles/tileset.json"},{"type":"WMS","url":"https://wms.craig.fr/administratif"}]}]

@landryb
Copy link
Collaborator

landryb commented Mar 24, 2023

can layers elements be objects? I should follow the same system of other MapStore. Or maybe options can contain more settings (e.g. localized title?).

@offtherailz just curious, what do you have in mind ? setting layerOptions ? afaik, so far the layers array can only contain a list of strings but if it's possible to set some layer options that's .. interesting.

@allyoucanmap
Copy link
Contributor Author

can layers elements be objects? I should follow the same system of other MapStore. Or maybe options can contain more settings (e.g. localized title?).

I should be as much as possible consistent with the rest of the layers.

The layer element is a string used initially as text in the filter for the .textSearch method of the catalog and then used as identifier to match the specific layer in the catalog. The 3D tiles does not need filter or search text because the record is already one so I simply used this information as default title. I don't think an object is really needed for the moment

@offtherailz
Copy link
Member

can layers elements be objects? I should follow the same system of other MapStore. Or maybe options can contain more settings (e.g. localized title?).

@offtherailz just curious, what do you have in mind ? setting layerOptions ? afaik, so far the layers array can only contain a list of strings but if it's possible to set some layer options that's .. interesting.

The idea is to try to see if we can respect as much as possible the same schema and functionalities of the WMS layers so we can expect more or less the same interface and functionalities.

What we implemented now works well and satisfies the requirements we have, I just asked to @allyoucanmap to check this point.

@allyoucanmap
Copy link
Contributor Author

@offtherailz just improved a bit the parsing of the title and added the related doc, see
432845f

@offtherailz offtherailz self-requested a review March 24, 2023 09:46
@allyoucanmap allyoucanmap merged commit 450b0e5 into geosolutions-it:master Mar 24, 2023
@allyoucanmap
Copy link
Contributor Author

@ElenaGallo please test on dev and let me know if I can backport on branch 2023.01.xx, thanks

@ElenaGallo
Copy link
Contributor

@allyoucanmap the rest passed on DEV, @landryb please backport to 2023.01.xx. Thanks

@landryb
Copy link
Collaborator

landryb commented Mar 24, 2023

@allyoucanmap the rest passed on DEV, @landryb please backport to 2023.01.xx. Thanks

i'm not sure i should be the one backporting, same for #9048, but can do if needed :)

@allyoucanmap
Copy link
Contributor Author

@landryb don't worry, I'll do the backport of this

allyoucanmap added a commit to allyoucanmap/MapStore2 that referenced this pull request Mar 29, 2023
@allyoucanmap allyoucanmap removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Mar 29, 2023
tdipisa pushed a commit that referenced this pull request Mar 29, 2023
… parameters (#9021) (#9060)

* #8055 allow to load 3dtiles tileset via viewer parameters (#9021)

Co-authored-by: Lorenzo Natali <[email protected]>

* fix tests

---------

Co-authored-by: Lorenzo Natali <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow to load 3dtiles tileset via viewer parameters
5 participants