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

Unify way to check readable blender files. #5153

Merged
merged 2 commits into from Jul 1, 2023

Conversation

feuerste
Copy link
Contributor

@feuerste feuerste commented Jun 22, 2023

Previously the InternReadFile logic was much more restrictive than the CanRead logic, but able to also parse compressed files. With this change the behavior is unified.

@feuerste feuerste force-pushed the blender_can_read branch 2 times, most recently from 95b6d88 to fe9d48d Compare June 26, 2023 07:14
@@ -1338,4 +1295,55 @@ aiNode *BlenderImporter::ConvertNode(const Scene &in, const Object *obj, Convers
return node.release();
}

std::string BlenderImporter::ParseMagicToken(std::shared_ptr<IOStream>& stream) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not pass shared_ptr via reference. Either pass by value or pass the raw pointer.

Copy link
Contributor Author

@feuerste feuerste Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we usually shouldn't pass smart pointers by reference.
However, in this specific case the stream is an input/output parameter, as it can internally be reset, if the file is compressed, and will be reused outside of this function. So passing by value wouldn't work. Passing by raw pointer would require us to clean up the old memory, so the whole point of using smart pointers is lost.

Of course if you wished we could rewrite more of the code here, but I didn't want to touch too much of the original logic (besides converting exceptions to strings).

@feuerste feuerste force-pushed the blender_can_read branch 2 times, most recently from 0df49d0 to 808869e Compare June 28, 2023 06:04
@feuerste
Copy link
Contributor Author

@turol Would you mind to restart the workflows and take another look? Thanks a lot!

@turol
Copy link
Member

turol commented Jun 28, 2023

I still don't like the use of reference parameters. I think the correct solution would be to return a std::variant with either an error or the new buffer/stream. This might require C++ 17 however.

Copy link
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine!

@feuerste
Copy link
Contributor Author

I still don't like the use of reference parameters. I think the correct solution would be to return a std::variant with either an error or the new buffer/stream. This might require C++ 17 however.

@turol I removed all reference arguments and created a helper struct to contain the stream, optional uncompressed data, and error. This is under the assumption that we still want to compile with < c++17 and have no variant support.
I think it is much cleaner now. PTAL!

@turol
Copy link
Member

turol commented Jun 29, 2023

If you're going to rebase it you might as well squash the whole thing into one commit.

@feuerste
Copy link
Contributor Author

If you're going to rebase it you might as well squash the whole thing into one commit.

Done.

@kimkulling kimkulling merged commit e4cac7d into assimp:master Jul 1, 2023
11 checks passed
@kimkulling
Copy link
Member

Merged, thanks a lot for your contribution.

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.

None yet

3 participants