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

Why are the Material Object members optional? #90

Closed balazsdukai closed 2 years ago

balazsdukai commented 2 years ago

v1.1 The Material Object must have member name to identify it, and may have several members to define it, as per X3D.

What is the reason behind making the material properties optional? If I only have a name in the material object, but the amientIntensity, diffuseColor etc are missing, how can I render the material?

hugoledoux commented 2 years ago

I guess the idea what that eg transparency is not mandatory, you can still display the data without... and if only diffuse than most code would produce something.

You think they should all be mandatory? It's better in a way, but too strict I find.

I don't have a strong opinion on this, as you can see!

balazsdukai commented 2 years ago

I think since the X3D specs are used + isSmooth, we should follow the X3D specs. Thus make all X3D fields mandatory, to make sure that the material can be rendered fully, if it is stored on the CityObject. I guess isSmooth can be optional, since it's not part of X3D.

hugoledoux commented 2 years ago

The properties were taken from CityGML v2.0.0 specs, see below.

These are not mandatory, but they have default values... which is weird, no idea where one would encode those?! In the software, but how? I'm confused here.

So I agree with your proposal, all mandatory except isSmooth

CleanShot 2021-10-13 at 13 44 07@2x
hugoledoux commented 2 years ago

@clausnagel maybe you can help us here?

We thought it was intelligent, but now I see that the official Den Haag dataset (https://3d.bk.tudelft.nl/opendata/cityjson/1.0/DenHaag_01.json) that you converted has only one of the properties...

hugoledoux commented 2 years ago

okay, spoke to some people and better to leave them as non-mandatory since with a few fields often a software can display the data

96001b4759cacc37c39d44c3d02921981f0771bf

clausnagel commented 2 years ago

Sorry for not responding earlier. Keeping the properties optional sounds good to me. Maybe the CityJSON specification should then also define default values like in the X3D specification referenced by @balazsdukai or in CityGML 2.0 (which uses the same default values). I think JSON schema supports default values, but this could also just be added to the appearance chapter in the specification document.

In the CityGML-to-CityJSON conversion with the recent version of citygml-tools, the properties are only created if they actually exist in the CityGML file. I could also change the behavior so that they are created with the default values as defined in CityGML. As long as CityJSON does not define default values, this might even be the preferred behavior.

hugoledoux commented 2 years ago

Thanks for the reply @clausnagel, indeed JSON Schemas have defaults but I always found these weird since they only make sense for the code generating an instance... And a default for a colour is perhaps not what we want, for transparency and shininess it makes sense though. I removed the mandatory, back to what v1.0 was thus

If you configured citygml4j to use the default that would be great 👍 Any estimate on a release with v3 support btw?

clausnagel commented 2 years ago

I see and understand your point regarding default values. I am not an X3D expert. Is there a minimum set of attributes that must be present to be able to colorize a surface? If so, these attributes should maybe be mandatory then.

Yes, I will adapt the citygml-tools implementation to write the default values in the CityJSON output. Should be simple. The work on citygml4j supporting CityGML v3 is still ongoing (https://github.com/citygml4j/citygml4j/tree/citygml3-devel). The GML schemas are not finished yet, and I expect some minor changes with the next draft release of the schemas. I hope I can release end of this year latest.

Once I have implemented support for CityGML v3, my next todo is to implement support for CityJSON v1.1 and especially to be able to convert between the two.

hugoledoux commented 2 years ago

Is there a minimum set of attributes that must be present to be able to colorize a surface? If so, these attributes should maybe be mandatory then.

it seems it depends on the software implementation... so I'll leave it as is for v1.1.0