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

Unexpected handling of generator during merge #1363

Closed
javagl opened this issue Apr 21, 2024 · 3 comments · Fixed by #1375
Closed

Unexpected handling of generator during merge #1363

javagl opened this issue Apr 21, 2024 · 3 comments · Fixed by #1375
Labels
bug Something isn't working package:core
Milestone

Comments

@javagl
Copy link

javagl commented Apr 21, 2024

This might not be considered to be an "issue", but ... I stumbled over this, and find it at least unexpected: When calling merge, then the asset.generator of the target document is apparently just overwritten with the one from the merged-in document.

To Reproduce

Run this:

import { Document } from "@gltf-transform/core";

async function run() {
  const documentA = new Document();
  const documentB = new Document();
  const documentC = new Document();
  documentA.getRoot().getAsset().generator = "Generator A";
  documentB.getRoot().getAsset().generator = "Generator B";
  documentC.getRoot().getAsset().generator = "Generator C";
  documentA.merge(documentB);
  documentA.merge(documentC);
  console.log(documentA.getRoot().getAsset().generator);
  console.log(documentB.getRoot().getAsset().generator);
  console.log(documentC.getRoot().getAsset().generator);
}

run();

The output will be

Generator C
Generator B
Generator C

Expected behavior

That's the tough one 🙂 The generator is basically a "free-text" field without any specified meaning. But roughly speaking, I'd expect that ~"no information is lost". A simple and straightforward solution could be just just append the merged-in generator to the one that is already present, yielding

Generator A, Generator B, Generator C
Generator B
Generator C

in the above example. But... with the obvious caveat that the output should't be

Generator X, Generator X, Generator X, Generator X, Generator X, Generator X, Generator X, Generator X

when merge-assembling one target document from 8 partial input documents.

Regardless of what the solution could or will be, I thought it might be worth bringing this up...

Versions:

  • 3.10.1
  • Environment: All
@javagl javagl added the bug Something isn't working label Apr 21, 2024
@donmccurdy donmccurdy added question package:core and removed bug Something isn't working labels Apr 23, 2024
@donmccurdy donmccurdy added this to the 🗄️ Backlog milestone Apr 23, 2024
@donmccurdy
Copy link
Owner

donmccurdy commented Apr 23, 2024

I'm not aware of any precedent for comma-separated generator strings, and I'm hesitant to invent one... I think this is kind of what XMP's 'ingredient' concept was made for, but it's complicated and subjective enough (discussed in #1367) that I don't think glTF Transform should try to do that in merge() automatically.

Is this a problem for your use case? My hunch is that your application knows better than glTF Transform can know, whether one generator string should win, or whether concatenating them is meaningful... It might be best to assign the generator string directly in the script above.

In terms of the default behavior — I'm tempted to say that the original generator string on the document should be kept, and not modified in a merge, which feels more semantically defensible than last-document-in-wins.

One could also argue that the generator should be "glTF Transform". ;)

@javagl
Copy link
Author

javagl commented Apr 23, 2024

The comma-separated list was just an overly suggestive attempt to "retain all information" (with the obvious caveats).

I'd also agree that keeping the generator of the target document when merging in others could make more sense. (And that's what I assumed to happen ... but... fixed it...). Justifications for that could be...

  • the order in which documents are merged into a target should not matter
  • the document that others are merged in is usually generated by glTF-Transform (if it was created from scratch)

But I don't have a strong opinion about the solution. Knowing what the behavior is allows users to set the generator as they see fit, and I just wanted to bring it up. (So the issue could be closed if there's no strong reason to change anything).

@donmccurdy
Copy link
Owner

donmccurdy commented Apr 27, 2024

Proposed solution:

tl;dr - a.merge(b) is replaced by mergeDocuments(a, b). The generator string of the target document will be kept when merging, along with other root-level properties. For more granular control, the new copyToDocument(target, source, sourceProperties) function may be used instead.

@donmccurdy donmccurdy modified the milestones: 🗄️ Backlog, v4.0 Apr 27, 2024
@donmccurdy donmccurdy added bug Something isn't working and removed question labels Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working package:core
Projects
None yet
2 participants