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

Clarify how to nest children of CityObjects #49

Closed
balazsdukai opened this issue May 7, 2019 · 9 comments
Closed

Clarify how to nest children of CityObjects #49

balazsdukai opened this issue May 7, 2019 · 9 comments
Labels
Milestone

Comments

@balazsdukai
Copy link
Member

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 ( #48 ) to support this by making the description and example more explicit. Please correct me if I'm wrong.

@liberostelios
Copy link
Contributor

liberostelios commented May 20, 2019

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

hugoledoux commented Jun 27, 2019

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
Copy link
Member

Re: versioning, the hash function could simply skip parents

@clausnagel
Copy link
Contributor

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
Copy link
Member

cjio already validate these!

@kenohori kenohori added this to the 1.0.1 milestone Jul 3, 2019
@hugoledoux
Copy link
Member

d345244

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants