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

Properly clear the model before loading #493

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

ptc-tgamper
Copy link
Contributor

@ptc-tgamper ptc-tgamper commented Jul 4, 2024

Not all, but only some, model fields were cleared before parsing.
As one can pass an existing model to the load functions, we should make sure to completely wipe the model before parsing.

@syoyo
Copy link
Owner

syoyo commented Jul 4, 2024

I have no idea why I coded manually initialize each members there.

I think it is better to use default constructor to initialize Model. How do you think?

(*model) = Model();

@marco-langer
Copy link
Contributor

clear() keeps the already allocated memory if the model was already used before. Default-constructing a new model releases all preallocated memory.

Maybe that was the original intention here?

@ptc-tgamper
Copy link
Contributor Author

I just followed the existing code and completed it. But I can simply default construct a new model, that makes the entire block into a one-liner.

@ptc-tgamper
Copy link
Contributor Author

Done

@syoyo
Copy link
Owner

syoyo commented Jul 5, 2024

clear() keeps the already allocated memory if the model was already used before. Default-constructing a new model releases all preallocated memory.

Maybe that was the original intention here?

Memory efficiency was not considered.

TinyGLTF was first written for C++03, and I think at that time I don't trust myself I put all initializations in the constructor.

Now, in C++11, we can initialize member variable in member declaration, so its safe to assume a default constructor initializes all members correctly.

@syoyo syoyo merged commit fea6786 into syoyo:release Jul 5, 2024
8 checks passed
@syoyo
Copy link
Owner

syoyo commented Jul 5, 2024

Thanks! Merged!

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