cityjson / specs

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

Clarify how to nest children of CityObjects #49

Closed balazsdukai closed 5 years ago

balazsdukai commented 5 years ago

It was not completely clear for me what can I expect in the children array of a 1st-level CityObject. Whether the array contains all the COs. that are related to it (children of children), or just those that are one level below (only immediate children).

My interpretation of the current specs is that the second is true. I opened a PR ( https://github.com/tudelft3d/cityjson/pull/48 ) to support this by making the description and example more explicit. Please correct me if I'm wrong.

liberostelios commented 5 years ago

This is related to #38. As soon as there is a "parents" property in CityObjects it's not clear how important is the "children" one. I think "parents" is redundant and should be gone, therefore there will be no ambiguity on how "children" works: it will only contain objects of one level below.

kenohori commented 5 years ago

I think children should only contain immediate children and parents should only contain immediate parents, but this is indeed not clear from the spec. @hugoledoux, merge?

@liberostelios, redundancy is okay IMO. Both use cases (parent->children and child->parent) are common. No need to force the user to compute it. See also #28.

hugoledoux commented 5 years ago

I'll merge but only when I make v1.0.1, waiting for other small bugs to be discovered.

redundancy: let's think about it and see if others have an opinion too.

clausnagel commented 5 years ago

Personally, I would always advise against redundancy unless there are very good reasons for it. As outlined in #38, I think parents could be easily removed. Since a strong assumption of CityJSON is that files are to be read in main memory for processing, the parent relationship can always be rebuild from the children information on the fly.

And imho parents is neither required for identifying 2nd-level objects while parsing. For the predefined CityObjects in CityJSON it is clear whether they are top-level. And CityObjects defined by an Extension are required to carry a toplevel attribute.

Am I missing an additional benefit of having the parents property?

hugoledoux commented 5 years ago

We can surely do without parents in 2nd-level objects, I don't think you are missing anything @clausnagel .

However, the question is whether we want to leave the reconstruction of the relationships to the developers. I mean, it's easy to do but it means that for many many tasks the developer has to do this, while we can easily store it. Yes there's redundancy, but it's for a good cause, no? We flattened the data model, and by removing parents we put the burden on the developers...

I coded some functions in cjio and knowing what is the parent of a given object is super useful to have built-in the object. If left to the developer then it's always a check in an auxiliary data structure.

So unless there are good reasons (versioning was a potential one, see #32 ) I'd favour sticking with redundancy.

kenohori commented 5 years ago

Re: versioning, the hash function could simply skip parents

clausnagel commented 5 years ago

I reckon everything is fine as long as children and parents are consistent. Assuming this, both relationships are in fact very useful when consuming the data. The producing system has to ensure this consistency when creating CityJSON datasets. So, some developer will have to write some extra code. But, well, in the end this is also not too difficult...

Maybe cjio could check the consistency of children and parents when validating a dataset?

hugoledoux commented 5 years ago

cjio already validate these!

hugoledoux commented 5 years ago

d3452444dc30599721059dfaa14b4767c797acfe