-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
KHR_xmp_json_ld #1893
KHR_xmp_json_ld #1893
Conversation
…ON-LD within KHR_xmp_ld.
I hate to say it, but when I first read this in the sans-serif title font, I read it as In sans-serif, KHR_xmp_ld and KHR_xmp_Id look almost identical. KHR_xmp_ld In monospace, the difference is clearer, as one serif is missing (with the monospace font on my system at least).
Not sure if there's a good solution to this. Might it be worth breaking convention and using KHR_xmp_LD? |
That's a great point. I'm in favor of breaking convention here for clarity. If there are no protests I will make this change later today. |
While reading the latest published version of the ISO spec this morning it dawned on me that we may be able to allow the @lexaknyazev, what do you think? Interested to hear your take on this. |
@emackey Imo, the question comes down to whether "a glTF extension name" is a strong enough context that is expected to always use lower-case (with deprecated spec-gloss being the only exception) thus implicitly resolving the issue. We could also make the whole string to use upper-case given that both XMP and LD are abbreviations. |
What is the written phrase, "XMP LD", intended to mean? Or more specifically, is it common to use the abbrevation "LD" with that meaning anywhere except as part of the full "JSON-LD" acronym? I would be tempted to expand the name for clarity, rather than breaking convention, with something like |
The initial idea was to signify the support of the JSON-LD for XMP ISO spec somewhere in the name, but since ISO is trademarked we can't use something like That said, on the 3D Commerce call today it was brought up that since our convention is to do all lowercase after the vendor prefix, is this really an issue? While the uninitiated may make this mistake, they'll only make it once. Additionally, I'd expect most use cases are going to be some tool actually writing the metadata to the file. With the additional context of expanding the name out to |
Do we have some updated sample models that implementors can test? |
After going through the requirements of this extension, it seems that some of them could be incompatible with ISO and XMP specs. Ordered arrays This spec:
ISO:
Language Alternatives
This spec directly stores language alternative JSON objects without using |
Honestly, with The language alternatives restriction is a potential issue, but the ISO spec seemingly has conflicting examples of what is acceptable. In section 4.5.5.3 it provides the provided example shows an unordered list in the provided XML with 4 list items, and in the JSON-LD these are provided in a I'm fairly uneasy about going down this path with Language Alternatives as I'm afraid it's going to make using just JSON with this extension far more difficult. However, I'm not sure what else we could do to rectify the situation in this case. I don't believe it's acceptable at all for us to say to people, "yes, you will need a JSON-LD parser as well," as in that case we've not solved the problem for smaller users who don't have any interest or need for JSON-LD in their tech stack. When we ran our survey it was about a 50/50 split of respondents, so whatever we can do to meet the needs of both is ideal. Perhaps if we document this sufficiently on how to handle Language Alternatives, that will be enough. |
Should both ways of storing unordered collections be supported (e.g. value objects with
That section describes mapping of
I'm not sure how to quantify "far more difficult" here. The most annoying thing would be to include |
I would say yes, lets support both You're right about 4.5.5.3, thanks for setting me straight. Regarding the difficulty with the However, as you said, we can't claim ISO compatibility at all without supporting the |
What do you think about requiring |
I'm on board with that. It's relatively painless to deal with for implementers and we gain quite a bit from it. |
We have adapted an existing xmp asset to the new version. Is the extension used correctly? |
I just merged in a PR with the edge cases. You can find them in the examples/edge-cases path in the glTF-Metadata-CLI repository. @UX3D-hohenester I took a look at that zip and that does look correct to me. I'm hoping to have the migration feature in the CLI tool released sometime this or next week. |
@weegeekps Can the samples be moved to a separate PR on the glTF-Sample-Models repo? We don't want sample models directly in the spec repo anymore. Thanks! |
@emackey Yes. I've just done so and it can be found at KhronosGroup/glTF-Sample-Models#291. Please let me know if there are any changes that need to be made to it. I didn't include a screenshot as the important bit is the metadata inside, not the visual appearance. |
More nits.
|
Couple more notes:
|
Good nits, @lexaknyazev.
In the original @MiiBond Do you have contact with Leonard Rosenthal or anyone else on the XMP side at Adobe that might be able to help us answer this question? |
I'm here :). Let me go through the comments... Let me start by saying that the spec is currently on way to publication, but we are going to see if we can pull it back to resolve some of these excellent findings!!
You are absolutely correct - this isn't covered and the examples are inconsistent. Ugh! https://github.com/ISOTC171SC2/XMP-JSON-LD/issues/3
Can you say more about this? Not understanding the issue here...
All good points here. Let me know if you need me to review anything on your side. |
That refers to some leftover descriptions in our spec from non-JSON-LD version of the extension. |
@lrosenthol I'm wondering if we should hold off on ratification ourselves until this is resolved on your end. I'd like for us to get this right. If you manage to pull it back, do you have an idea of how long it will take to get a draft that resolves these issues?
Just an FYI: that link seems to be inaccessible. Perhaps it's a private repository? |
@weegeekps that's up to you folks, but I don't see it as necessary. I will make the updates this weekend and that should mean that we don't lose any time... |
@lrosenthol has there been any progress on your updates? We have decided to wait until we can get a glance at the next draft of the ISO spec so we can make sure we are fully compliant. |
@lrosenthol, have you made any progress regarding your updates to the ISO spec? Anything you can share us regarding what has changed would be helpful and much appreciated for getting this extension moving again. |
@lrosenthol @MiiBond Could we get some guidance on how we should handle Booleans, Integers, and Reals? I know you are updating the ISO spec now to fix this issue, but there very strong chance that we will soon be normatively referenced by others very soon, and we'd like to wrap this up as soon as possible given that. If we can get an answer to this one question, I think we can confidently go forward with ratification on our side. Thank you! |
I had an email conversation with @lrosenthol and we have the answers we need. The decision is to use the JSON types for Booleans, Integers, and Reals, which means there are no normative changes left. I am working on an update that will shore up our last nits and typos, and we are hoping to bring this to a ratification vote soon. |
I went through and finally pushed my updates based on your last nits and the decision on the Boolean, Integer, and Real issue, @lexaknyazev. I have a few questions bout how to handle a few of these.
Could you expand on this a bit? I'm not quite sure I follow.
Do we have a suggested approach for dealing with this? I guess just normalize based on the actual namespace URI? Seems obvious but I want to confirm, I imagine implementers will want some direction on this.
Are you talking about the root |
@lexaknyazev, @emackey, now that this is ratified, I'd like to get this merged in by Monday prior to the wider announcement. Do either of you (or anyone else) have any final comments before I merge? |
I'm not an XMP user, but from a glTF perspective this looks fine to me. Actually I think we should have merged this when it entered ratification. I'll merge it now. |
I have one question. Do we allow multiple reference of xmp_json_ld from one structure (mesh, node, etc.) For example, as below. Or this case should handled at the packet definition, aka the person who define the extension should merge the json-ld if there is this case, and always keep reference contains one extension? thank you "materials" : [ |
JSON doesn't allow duplicate keys in one object, so it's not supported even if we wanted to. The forum might be a better place to ask these types of questions. |
Opening up the discussion for the new JSON-LD compatible version of KHR_xmp.
Some context: it's been identified that there are some inconsistencies in the original specification around KHR_xmp and it's relationship to JSON-LD. This new version of the extension is an attempt at clarifying and solidifying the relationship between KHR_xmp and JSON-LD. In regards to XMP, JSON-LD is important as the XMP team are currently nearing completion of a new ISO standard defining JSON-LD serialization of XMP. We'd like to align more closely to that while also providing additional guidance and restrictions within KHR_xmp in order to ensure both JSON and JSON-LD parsers can properly handle the data.
In short, JSON-LD won't be required or recommended, but if you want that extra control and validation within your KHR_xmp packets, you can have it.
This is largely an in progress pull request at the moment. Here are a list of tasks I've identified, and their progress.@context
within each packet.This should be ready for discussion and review.