Skip to content

Conversation

@riccardobl
Copy link
Member

@riccardobl riccardobl commented Nov 9, 2023

The purpose of this PR is to deprecate TangentBinormalGenerator

  • Deprecate TangentBinormalGenerator in favor of MikktspaceTangentGenerator
  • Add a bit more context to the comment in MikktspaceTangentGenerator
  • Remove the part about it being experimental, since at this point it has been tested and patched enought to be considered out of this stage
  • Add generate(Mesh) to match TangentBinormalGenerator
  • Move debugging utils from TangentBinormalGenerator into TangentUtils to be used with MikktspaceTangentGenerator
  • Replaces the usage of TangentBinormalGenerator in all the examples and tests (except the Parallel Tangent test since out mikktspace doesn't have a parallel implementation).

This PR needs to be tested

Copy link
Contributor

@codex128 codex128 left a comment

Choose a reason for hiding this comment

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

Just a typo on a javadoc link needs fixing.


/**
*
* @deprecated This is an outdated and non-standard method. Please use @{link MikktspaceTangentGenerator}
Copy link
Contributor

Choose a reason for hiding this comment

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

Change @{link to {@link.

@stephengold stephengold added this to the Future Release milestone Nov 14, 2023
@capdevon
Copy link
Contributor

capdevon commented Feb 6, 2024

Hi guys, what is the status of this PR ? Is it complete ?

@yaRnMcDonuts yaRnMcDonuts modified the milestones: Future Release, v3.8.0 Dec 28, 2024
@yaRnMcDonuts
Copy link
Member

yaRnMcDonuts commented Dec 28, 2024

Hi guys, what is the status of this PR ? Is it complete ?

After reviewing the code in regards to its functionality (not for formatting or anything else), I would say it is complete and looks ready to be merged. I have also been using the MikktSpaceTangentGenerator extensively with a fairly wide variety of models in my own project and editor for at least 2 years now, so I am personally confident in its reliability.

I'd also like to try to merge this to get it into the 3.8 alpha release that will be out soon. That way we can get jme users testing these changes as early in the alpha / beta process as possible.

But (since this PR has been sitting for a while) I will wait a few days before merging, in case @riccardobl has anything to add or wants to change anything before the PR is merged.

@yaRnMcDonuts yaRnMcDonuts merged commit 8dd829d into jMonkeyEngine:master Dec 31, 2024
@yaRnMcDonuts
Copy link
Member

yaRnMcDonuts commented Dec 31, 2024

I merged this PR so it will be in the first 3.8.0-alpha1 release in the next day or so.

There were many engine examples updated in this PR to use MikktspaceTangentGenerator and there will be a handful of test-cases that will need testing, so I think getting this into the alpha release asap is important.

I took a screenshots showing a list of every test-case / example that was changed in this PR, so that will hopefully make it easier for users to reference this list to test all of these in rapid succession:

image

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.

5 participants