You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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
varlayer={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){returnindex===0 ? -value : value;}),// Note: this seems to be superflous, as the geometry is translated when pivot is presentname: 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.
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?
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.
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.
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.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Mugen87
changed the title
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
lookAtworks 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
and I thought this would get more complaints.
I flipped the coordinates in webgl_loader_lwo.html file to match the removal of
-zand addition of-xI 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.