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

LWOLoader: Fix coordinate system conversion. #28029

Merged
merged 2 commits into from
Apr 2, 2024
Merged

Conversation

WORMSS
Copy link
Contributor

@WORMSS WORMSS commented Mar 30, 2024

Related issue: #26733

Description

fix: IFFParser.js now reads x as -x instead of z to -z. This now sets the geometry and pivot points in the correct location.

It seems the original comments in this file were correct, in stating switching the 'x', but the file was committed with the z being flipped instead.

Now with the z being drawn in the correct direction, code like lookAt works correctly (which is +z based) and the pivot points no longer need double negating to have the geometry drawn in the correct location (except still had a -z problem)

This fix now means no dirty hacks need to be post-applied after loading of the model.

A playground using this correction

This example shows both the minor fix from this PR, and also what exists in r163.

I am not a massive fan of having to break things out into variables, but it was either that or

var layer = {
  number: this.reader.getUint16(),
  flags: this.reader.getUint16(), // If the least significant bit of flags is set, the layer is hidden.
  pivot: this.reader.getFloat32Array( 3 ).map(function (value, index) { return index === 0 ? -value : value; }), // Note: this seems to be superflous, as the geometry is translated when pivot is present
  name: this.reader.getString(),
}

and I thought this would get more complaints.

I flipped the coordinates in webgl_loader_lwo.html file to match the removal of -z and addition of -x
I generated a new screenshot because even though they look identical to the human eye (except the grid helper) it seems 0.7% difference was too much to pass.

image

fix: IFFParser.js now reads x as -x instead of z to -z. This now sets the geometry and pivot points in the correct location.

This fixes mrdoob#26733
@WORMSS
Copy link
Contributor Author

WORMSS commented Mar 30, 2024

image

Do I need to build these screenshots on a specific computer?
Because apparently a screenshot built from its own code doesn't pass the test on the actions pipeline?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 31, 2024

Do I need to build these screenshots on a specific computer?

If your version of the screenshot still shows a diff, I'll generate the screenshot on my side after a merge. The reflections on the spheres are different because of the new transformation so we need a new screenshot in any event.

In any event, make sure to install the latest dev dependencies before generating the screenshot.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 31, 2024

Side note: This is a breaking change since all apps using LWOLoader have to potentially update the transformation of their scene. So if the PR is going to be merged, it has to be noted in the migration guide.

@WORMSS
Copy link
Contributor Author

WORMSS commented Mar 31, 2024

Do I need to build these screenshots on a specific computer?

If your version of the screenshot still shows a diff, I'll generate the screenshot on my side after a merge. The reflections on the spheres are different because of the new transformation so we need a new screenshot in any event.

In any event, make sure to install the latest dev dependencies before generating the screenshot.

I actually installed the dependencies especially for making the screenshot.
I used npm ci so it installs the exact versions of all the packages stated in the package-lock.json exactly.
So, they are technically not the latest versions of any of the dependencies to exist, but were the latest versions of approved by the repository.

@Mugen87 Mugen87 added this to the r164 milestone Apr 1, 2024
@Mugen87 Mugen87 merged commit 08fbd47 into mrdoob:dev Apr 2, 2024
10 of 11 checks passed
@Mugen87 Mugen87 changed the title [Loader] Fix for LWOLoader geometry correction LWOLoader: Fix coordinate system conversion. Apr 2, 2024
@WORMSS WORMSS deleted the patch-1 branch April 3, 2024 14:25
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