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

Added Texture.DEFAULT_ANISOTROPY #25015

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

WestLangley
Copy link
Collaborator

@WestLangley WestLangley commented Nov 25, 2022

Fixed #25010

Perhaps this is an acceptable solution...

@WestLangley
Copy link
Collaborator Author

Oh wait. I see @takahirox just suggested the same idea. :-)

@@ -304,5 +304,6 @@ class Texture extends EventDispatcher {

Texture.DEFAULT_IMAGE = null;
Texture.DEFAULT_MAPPING = UVMapping;
Texture.DEFAULT_ANISOTROPY = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, why the case differs between Object3D.DefaultUp/Object3D.DefaultMatrixAutoUpdate and Texture.DEFAULT_IMAGE/Texture.DEFAULT_MAPPING/Texture.DEFAULT_ANISOTROPY?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure but I think @mrdoob lately stated his preference for this nomenclature. Unfortunately, I don't find the discussion right now.

For the time being, it's sufficient when we adhere to naming conventions per file. At a later point, we can unify the syntax.

Copy link
Owner

Choose a reason for hiding this comment

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

It probably should be Object3D.DEFAULT_UP and Object3D.DEFAULT_MATRIXAUTOUPDATE...

@LeviPesin Are these the only ones that would need to be changed?

Copy link
Contributor

@LeviPesin LeviPesin Dec 8, 2022

Choose a reason for hiding this comment

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

I'm not sure but I think @mrdoob lately stated his preference for this nomenclature. Unfortunately, I don't find the discussion right now.

I think it was #23562.

@LeviPesin Are these the only ones that would need to be changed?

I think yes (static defaults are not used anywhere else, I think).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can make these changes for consistency if @mrdoob approves.

// from
Object3D.DefaultUp = /*@__PURE__*/ new Vector3( 0, 1, 0 );
Object3D.DefaultMatrixAutoUpdate = true;
Object3D.DefaultMatrixWorldAutoUpdate = true;

// to
Object3D.DEFAULT_UP = /*@__PURE__*/ new Vector3( 0, 1, 0 );
Object3D.DEFAULT_MATRIXAUTOUPDATE = true;
Object3D.DEFAULT_MATRIXWORLDAUTOUPDATE = true;

// from
Euler.DefaultOrder = 'XYZ';
Euler.RotationOrders = [ 'XYZ', 'YZX', 'ZXY', 'XZY', 'YXZ', 'ZYX' ];

// to
Euler.DEFAULT_ORDER = 'XYZ';
Euler.RotationOrders // wait. why is this here? leave as-is -- or deprecate it?

// as-is
Texture.DEFAULT_IMAGE = null;
Texture.DEFAULT_MAPPING = UVMapping;
Texture.DEFAULT_ANISOTROPY = 1;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Euler.RotationOrders // wait. why is this here? leave as-is -- or deprecate it?

I vote to remove it. Possible euler rotation orders should be clear without defining them in an array.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 25, 2022

I've also thought about adding this change when reading #25010 but I'm afraid this opens the door for adding even more global defaults. Devs could argue to add the same for other parameters and it's hard to argue against this if we merge this PR.

To me, I think the presented workaround in #25010 (comment) sounds sufficient. An additional traverse should not be noticeable in terms of loading time.

@takahirox
Copy link
Collaborator

Another solution may be...

  • Test .anisotropy > 1 and check visual quality and performance
  • If visual quality benefit is big and performance penalty is small, change the hardcoded default .anisotropy to for example 4.

@hybridherbst
Copy link
Contributor

Regarding "opening tthe door for more globals", out of curiosity, I think it would still be on a case-by-case basis, right? And in some cases it may well make sense. Anisotropy as an example of a typical per-app default is kind of a good example for something that could be a global.

In Unity Anisotropy is also one of the few quality globals:
image
(it has options "Disabled", "Per Texture", "Forced On")

Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

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

Okay, I'm convinced. The addition indeed make sense in this case.

@Mugen87 Mugen87 added this to the r147 milestone Nov 26, 2022
@Mugen87 Mugen87 merged commit 1d860e0 into mrdoob:dev Nov 28, 2022
@WestLangley WestLangley deleted the dev-texture_anisotropy branch November 28, 2022 17:52
@0b5vr 0b5vr mentioned this pull request Dec 7, 2022
12 tasks
0b5vr added a commit to three-types/three-ts-types that referenced this pull request Dec 19, 2022
0b5vr added a commit to three-types/three-ts-types that referenced this pull request Dec 19, 2022
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.

Ability to control texture.anisotropy from GLTFLoader
6 participants