cityjson / specs

Specifications for CityJSON, a JSON-based encoding for 3D city models
https://cityjson.org
Creative Commons Zero v1.0 Universal
107 stars 25 forks source link

should "appearance" be inside a CityJSONFeature or in the "first line" (global)? #166

Closed hugoledoux closed 10 months ago

hugoledoux commented 1 year ago

Right now we have inside each CityJSONFeature, but since we have "geometry-templates" global (and "transform" and "metadata") then it would make more sense to do this too for "appearance", no?

clausnagel commented 1 year ago

I think at least appearances affecting the "geometry-templates" should be stored together with the templates in the first line. Otherwise they would have to be duplicated for every CityJSONFeature where a template is used.

balazsdukai commented 1 year ago

I also vote for global appearances. I cannot imagine a realistic (and common) scenario where one would need a different appearance for each CityJSONFeature.

clausnagel commented 1 year ago

But it would still be possible to store appearances affecting just a single city object inside the corresponding CityJSONFeature?

hugoledoux commented 1 year ago

If we allow both it gets very complicated for the client. The texture/material is a 0-based index in "appearance", so which one should they use? If a stream starts to mix-and-match local and global it gets a bit tricky, no? I'd favour all local or all global.

Why would you want to mix them actually?

clausnagel commented 1 year ago

Ah, ok , I see the issue with the 0-based index.

I think creating a CityJSONFeature dataset becomes more complicated when you have to use global appearances. I would like to be able to create the CityJSONFeature dataset in a streaming fashion. For example, when reading a CityGML dataset top-level feature by top-level feature, you currently can simply convert every CityGML top-level feature to a CityJSONFeature and write it to the stream. Works nicely for arbitrarily huge datasets with low memory footprint. The same streaming approach works when reading top-level features from another input source such as a database.

When global appearances are required, you first have to scan the input source for all appearances associated with the city objects to be written to the CityJSONFeature dataset. Afterwards, you have to parse/query the input source a second time to be able to write to the stream. And you have to keep some lookup table for the appearances in main memory. That's why I would prefer to also have local appearances for each CityJSONFeature.

Creating the dataset would be easier if you could also store the global information in the last line instead of the first line. In this case, it would be sufficient to parse/query the input source only once and create the lookup table on-the-fly. But then, it might be more complicated for the client to consume the dataset?

clausnagel commented 1 year ago

Can't you just specify that every "appearance" is only valid for the line in which it is stored? Then the "appearance" in the first line is only used for the global "geometry-templates". And every CityJSONFeature may have its own additional "appearance" which is only valid for geometries stored inside the same CityJSONFeature. I mean, in CityJSON 1.1 you also have individual "appearance" properties for each CityJSONFeature and they all use a 0-based index. The only new thing would be that the first line can also have an appearance for the templates.

hugoledoux commented 1 year ago

Creating the dataset would be easier if you could also store the global information in the last line instead of the first line. In this case, it would be sufficient to parse/query the input source only once and create the lookup table on-the-fly. But then, it might be more complicated for the client to consume the dataset?

This is not prescribed by CityJSON, accessing the "metadata"/global-information is left to the client. We wrote the 1st line, but if you want to have the last line for your application it's fine. We made a small change last week to mention that other means (such as OGCAPIF) could be used.

cjio outputs with first line is you export to JSONL, but I guess the last line could be a flag, if you need that

clausnagel commented 1 year ago

Ok, thanks for your clarification that the last line is already an option as well.

What about having the "appearance" property in every line, so for the global templates in the fist/last line and for the geometries of each CityJSONFeature in the corresponding line? Since it is already implemented this way for the separate CityJSONFeatures, I guess this small change would solve the issue, right?

balazsdukai commented 1 year ago

I've made a short experiment for measuring the size of the appearance object if we have it as global info, using the 3D BAG data.

https://gist.github.com/balazsdukai/9b594da08264cc436093e665dd034731

In short, I was wrong, and it is not feasible to have the appearances global. The texture related objects add too much overhead and I did not consider this before. Assuming that we have a fully textured building data set with about 10mio objects, served as CityJSONFeatures through a webservice, the client would need to pull over 10Gb (close to 18Gb) appearance data upfront.

clausnagel commented 10 months ago

I wanted to start implementing support for CityJSON 2.0 in citygml4j, but found this issue still to be open. I need clarification on where to store appearances in a CityJSONFeature dataset.

I have created a small dataset containing trees with template geometries that have materials and textures: out.zip. When I convert this dataset to CityJSONFeature with the latest cjio, then unfortunately I don't get an answer because cjio skips the appearances: cjio.zip

I still think this should be the way to go:

Please confirm, thanks.

Update: This is how I think the JSONL version of my tree example should look like: out-jsonl.zip

hugoledoux commented 10 months ago

The v2.0.0 specs specify that the appearances are per feature (based on https://github.com/cityjson/specs/issues/166#issuecomment-1520105419).

But indeed this is problematic for geometry-templates... Good you bring this point. The behaviour you describe (and implemented in out-jsonl.json is correct in my opinion: the "appearance" property can be added to the "first-line", and that will validate fine since it should be a CityJSON object.

And yeah cjio is a bit neglected at the moment, and that feature wasn't implemented yet...

hugoledoux commented 10 months ago

I guess I can close that issue now? If not please re-open it!

clausnagel commented 10 months ago

Thanks for your feedback @hugoledoux . Ok, will go ahead and implement it this way. Yes, agree that the issue can be closed and reopened on demand.

hugoledoux commented 6 months ago

Hello @clausnagel I didn't want to patch cjio (for different reasons) so I started a proper implementation in Rust. I just finished the first draft that supports everything (I think) and I have tested it a bit (but not thoroughly).

It's there: https://github.com/cityjson/cjseq

Do you have test files around, and do you have a full implementation in citygml4j?

clausnagel commented 6 months ago

Great work, @hugoledoux, thanks.

I think I have a full implementation in citygml4j for reading and writing CityJSONSeq but also not tested thoroughly. I typically use the Railway dataset for testing. Do you have a CityGML / CityJSON version of it? Otherwise I also use the typical open datasets on the web. There is some more customer data which I cannot share, unfortunately.

hugoledoux commented 6 months ago

I have railway yes, cheers.