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

EXT_structural_metadata - parent class #70

Open
wants to merge 1 commit into
base: 3d-tiles-next
Choose a base branch
from

Conversation

lilleyse
Copy link

@lilleyse lilleyse commented Dec 2, 2024

Adds the concept of a parent class that allows for class hierarchies, with some restrictions like no duplicate property IDs and no cyclical inheritance

@javagl would you mind taking a look?

@javagl
Copy link

javagl commented Dec 2, 2024

It's difficult to foresee all implications of this change. Some thoughts:


(EDIT: This PR has been split into two. What I wrote here originally now is in #71 (comment) )


The additional support for a parent class is not breaking in terms of existing data. Everything that worked before will just continue to work. But of course, clients will have to be updated to handle that new concept. And here, it's particularly hard to imagine what this might mean.

For example, looking at things like the existing MetadataClass in CesiumJS, a quick shot at supporting this would be to just add a parent: MetadataClass to that. But this means that such a class can no longer be parsed in a "standalone" fashion from a single JSON node. For example from a schema JSON like

 "classes" : {
      "classA" : { ... },
      "classB" : { ... }
}

it was trivial to just throw that "classA" object into a function like MetadataClass.fromJson. Now, that "classA" might declare "parent": "classB", meaning that it will be necessary to first generate the MetadataClass objects from all classes that do not define a parent, and store them in some repository, so that they can be looked up when generating the objects for classes that do have parents. (All this is possible, and maybe even "simple" - I'm just trying to think a few things through here...)

It may also require further differentiation for the properties: The properties should probably be all properties. But maybe there should be some sort of distinction between properties/ownProperties/inheritedProperties...?

When looking at things like propertiesBySemantic, the specification may have to update the description of semantic to say...

The semantic cannot be used by other properties in the class or any of its parent classes

And, more broadly: The EXT_structural_metdata is supposed to be an 'implementation' ("incarnation") of the general 3D Metadata specification. This new parent concept is not covered by this specification. And it raises the question of whether this concept should also be in the 3D Tiles metadata ...

@lilleyse
Copy link
Author

lilleyse commented Dec 2, 2024

And it raises the question of whether this concept should also be in the 3D Tiles metadata ...

I think it should, though I'm not sure if it can be done without adding a new extension.

@lilleyse lilleyse force-pushed the ext-structural-metadata-updates branch from 283f778 to 6946e4a Compare December 2, 2024 19:25
@lilleyse
Copy link
Author

lilleyse commented Dec 2, 2024

I moved the JSON encoding change to its own PR: #71

@lilleyse lilleyse changed the title EXT_structural_metadata updates EXT_structural_metadata - parent class Dec 2, 2024
@ColinKerr
Copy link

@javagl brings up some great points about the complications it adds. What problem do you want to solve by adding a parent class, maybe there are other concepts that could solve the same problem without the same complications.

@javagl
Copy link

javagl commented Jan 10, 2025

I hope it's OK to mention a "basic" reasoning for having such a parent concept, even if it may appear a bit oversimplified (maybe Sean wants to chime in and/or elaborate): There often is some sort of "hierarchy" or "inheritance" of metadata representations in the real world. For example, you could have metadata about a "car component", that includes a looong list of common properties,

  • some ID
  • material
  • weight
  • size
  • cost per unit
  • ...

and then you'd have metadata about an "engine", that contains all these properties, and additionaly a horsepower property, and metadata about a "seat", that contains all these properties, and additionally a coverColor property.

Right now, the only way to model this within EXT_structural_metadata is to have two metadata classes, engine and seat, and they have to share/duplicate the long list of common properties. Now imagine you have like 20 of these classes - that's a lot of duplication. And imagine, then you want to add a property like manufacturer to these common properties - you'd have to touch all the 20 classes.

The parent concept could make modeling some of these things far easier and more compact, At the price of the potential hassle and quirks that we know from "inheritance" in object-oriented programming languages 🙂 . I think that it's worth considering, but we should be careful about the implications.

@lilleyse
Copy link
Author

@javagl you summed it up pretty well. I think the discussion should move to the 3d-tiles repo. Even though I originally proposed this, would you mind opening an issue and summarizing the discussion there?

@javagl
Copy link

javagl commented Jan 15, 2025

@lilleyse I tried to extract/summarize this at CesiumGS/3d-tiles#786

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.

3 participants