Skip to content

Reverts the changes introduced by commit ad766cb in February 2022. - #5542

Merged
kimkulling merged 3 commits into
assimp:masterfrom
johannesugb:good-old-bitangent-code
Apr 29, 2024
Merged

Reverts the changes introduced by commit ad766cb in February 2022.#5542
kimkulling merged 3 commits into
assimp:masterfrom
johannesugb:good-old-bitangent-code

Conversation

@johannesugb

@johannesugb johannesugb commented Apr 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

ASSIMP's way of calculating bitangents before commit ad766cb was just fine, but unfortunately it was changed by that commit in February 2022, breaking compatibility to previous code.

It introduces two problems:

  • It orthogonalizes bitangents w.r.t. tangents (which is generally wrong for texture-mapped meshes).
  • It changes the handedness, which is arguably not totally wrong, but breaks compatibility to previous code which uses ASSIMP.

More detailed explanation why the old code was just fine is given in Q&A #5512

Suggestion: Just revert to the old code, which this pull request suggests.

@johannesugb johannesugb reopened this Apr 12, 2024
@johannesugb

johannesugb commented Apr 22, 2024

Copy link
Copy Markdown
Contributor Author

@kimkulling are these checks supposed to work or not? 🙃

@turol

turol commented Apr 23, 2024

Copy link
Copy Markdown
Member

Yes they're supposed to work. You forgot a ;

@johannesugb

johannesugb commented Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

Yes they're supposed to work. You forgot a ;

OMG, shame! 🙈

@johannesugb
johannesugb force-pushed the good-old-bitangent-code branch from 7fb2907 to 8143a16 Compare April 25, 2024 19:16
@kimkulling

kimkulling commented Apr 29, 2024

Copy link
Copy Markdown
Member

After reading all the explanations I do agree that we should use the original code.

kimkulling
kimkulling approved these changes Apr 29, 2024
View reviewed changes

@kimkulling kimkulling left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's fine.

@kimkulling
kimkulling merged commit 47ef796 into assimp:master Apr 29, 2024
@BrewTestBot BrewTestBot mentioned this pull request Jul 3, 2024
Merged
@BrewTestBot BrewTestBot mentioned this pull request Aug 30, 2024
Merged
kimkulling added a commit that referenced this pull request Jul 1, 2026
@johannesugb @kimkulling
Reverts the changes introduced by commit ad766cb in February 2022. (#…
13ec46d 
…5542)

* Reverts the changes introduced by commit ad766cb in February 2022.
Explanation why the old code was just fine is given in Q&A #5512.

* Added that missing ;

---------

Co-authored-by: Kim Kulling <kimkulling@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kimkulling kimkulling kimkulling approved these changes

Assignees

No one assigned

Labels

None yet

Projects

The Asset-Importer-Lib DevOps Board
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@johannesugb @turol @kimkulling