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

Fix the complete but empty example #207

Open
wants to merge 4 commits into
base: v2.0.2beta
Choose a base branch
from

Conversation

balazsdukai
Copy link
Member

The example of a complete but empty CityJSON object was invalid, because the templates and vertices-templates members are mandatory if geometry-templates is defined. Both members are arrays and they can be empty (but need to be present), just as in case of the root vertices member.

@hugoledoux
Copy link
Member

Nice catch.

But perhaps the question should be here: should those 2 properties be mandatory? For "appearance" nothing is mandatory...

And let's try to keep main branch the latest published, if we merge we should have a new branch v2.0.2 perhaps?

@balazsdukai balazsdukai changed the base branch from main to v2.0.2beta October 8, 2024 09:55
@balazsdukai
Copy link
Member Author

balazsdukai commented Oct 8, 2024

Hm okay, then I would say make vertices-templates and templates optional so that it is consistent with appearance and metadata, where all members are optional.
Even though, we cannot have a valid GeometryInstance without defining geometry-templates.vertices-templates and geometry-templates.templates , but the same is true for textures/materials on the geometries and the appearance property.

@balazsdukai
Copy link
Member Author

I made the two properties of the geometry-templates optional and I reverted to the initial example in the specs.

Additionally, added a sentence that the six main Metadata properties are optional, because it was not explicit I thought.

Could you check again @hugoledoux?

@balazsdukai
Copy link
Member Author

I just realized that we are also bound by compatibility here, because if we make the two fields optional, that's a breaking change, since newly generated files might not have the previously required fields...
So I reverted my changes back to the initial fix.

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

Successfully merging this pull request may close these issues.

2 participants