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

Feature: Add entries to existing tinygltf::Values #414

Open
wants to merge 2 commits into
base: release
Choose a base branch
from

Conversation

agnat
Copy link
Contributor

@agnat agnat commented Apr 15, 2023

Hi @syoyo,
how are you doin'? Thanks for the quick merge the other day. I continued exploring and came across another small issue with tinygltf::Value.

Consider a typical baking job that 1. reads an asset, 2. adds some extras (or extensions) and 3. saves the file. In this scenario we'd like to add entries to an existing object incrementally without replacing the whole object, because it may already contain entries from earlier stages (eg. the exporter). I couldn't find a (good) way to do that...

This PR adds a subscript operator to insert entries into objects. To maintain feature parity I also added a push method and subscript operators for arrays.

Let me know what you think...

agnat added 2 commits April 15, 2023 17:47
Add subscript operator for array and object type tinygltf::Values. Add a method Push(…) to append values to existing arrays.
@agnat
Copy link
Contributor Author

agnat commented Apr 15, 2023

We also could expose Resize() and maybe even Reserve() on arrays ... but I don't think that's necessary. I can add them if you want, though...

@syoyo syoyo added the trivial label Apr 16, 2023
@syoyo
Copy link
Owner

syoyo commented Apr 16, 2023

You are better first explicitly cast Value data to Array or Object

@agnat
Copy link
Contributor Author

agnat commented Apr 16, 2023

Apologies, but I don't understand what you mean. Could you elaborate or paste an example?

@agnat
Copy link
Contributor Author

agnat commented Jun 6, 2023

Hi @syoyo,
could you do me a favour and bonk the button on this? For me it's an actual issue and it would be great to get it landed.

@syoyo
Copy link
Owner

syoyo commented Jun 6, 2023

tinygltf::Value must be rewritten to make it type safe and remove assert in Value class for better security.

For example, ArrayLen, and const Value &Get(int idx) const(and other non-type safe API) must be removed.

It'd be better to provide type safe API to Value, such like:

bool GetAsArray(Value::Array *arr) {
   if (IsArray()) {
      if (arr) { (*arr) = array_value_; return true}
   }
   return false;
}  

// Do same thing for Value::Object type.

If you want to access array object in current Value API, you could do something like

if (v.IsArray()) {
  Array arr = v.Get<Value::Array>();
  for (item : arr) {
    ...
  }

  v = Value(arr); // update
}

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

Successfully merging this pull request may close these issues.

2 participants