Skip to content

Update writeDAE.cpp#1642

Merged
jdumas merged 1 commit intolibigl:masterfrom
FabienPean:patch-2
Nov 3, 2020
Merged

Update writeDAE.cpp#1642
jdumas merged 1 commit intolibigl:masterfrom
FabienPean:patch-2

Conversation

@FabienPean
Copy link
Copy Markdown
Contributor

  • Remove ambiguity (on windows) by using qualified calls everywhere.
  • Remove error with newer version of tinyxml2 which does not have XML_NO_ERROR (and was just an alias to XML_SUCCESS)

Check all that apply (change to [x])

  • All changes meet libigl style-guidelines.
  • Adds new .cpp file.
  • Adds corresponding unit test.
  • This is a minor change.

Remove ambiguity (on windows) by using qualified calls everywhere.
Remove error with newer version of tinyxml2 which does not have XML_NO_ERROR
@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Nov 3, 2020

Wow I didn't know people were still using the DAE writer...
Your PR is marked as a draft. Is that intended or is it ready for review?

@FabienPean
Copy link
Copy Markdown
Contributor Author

FabienPean commented Nov 3, 2020

Hi, I don't really use it :) It was mainly to test quickly and easily the feature libigl[xml] on vcpkg as I am currently trying to fix some things with the libigl port.

The PR was marked as draft until I could see that the CI passed without issue. Since it is the case, I pass it as ready for review.

@FabienPean FabienPean marked this pull request as ready for review November 3, 2020 23:36
@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Nov 3, 2020

I don't mind merging this, but just FYI the xml module is not actively used and might even be dropped in the future.

@jdumas jdumas merged commit fb3bd35 into libigl:master Nov 3, 2020
@FabienPean
Copy link
Copy Markdown
Contributor Author

No problem, I just did for the sake of completeness. Its future does not concern me anymore 😄

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.

2 participants