Skip to content

Add minFilter and magFilter to material component replacing npot#5717

Merged
dmarcos merged 1 commit into
aframevr:masterfrom
mrxz:texture-filtering
May 12, 2025
Merged

Add minFilter and magFilter to material component replacing npot#5717
dmarcos merged 1 commit into
aframevr:masterfrom
mrxz:texture-filtering

Conversation

@mrxz

@mrxz mrxz commented May 10, 2025

Copy link
Copy Markdown
Contributor

Description:
As discussed in #5706 the npot property was only intended for working with non-power-of-two textures in WebGL 1. It's being used in several places to indirectly set the minFilter to THREE.LinearFiltering. This PR introduces minFilter and magFilter properties allowing the filtering methods to be set directly and explicitly.

Changes proposed:

  • Add minFilter and magFilter properties to material component to set texture filtering.
  • Remove npot property from material component

@dmarcos

dmarcos commented May 11, 2025

Copy link
Copy Markdown
Member

PR ended pretty big

@mrxz

mrxz commented May 11, 2025

Copy link
Copy Markdown
Contributor Author

PR ended pretty big

Not really? The majority is the auto-generated attribute tables for primitives in the docs. Kind of unavoidable when material properties change.

The actual changes are minimal: npot out and mag/minFilter in:

 docs/components/material.md                       |  3 ++-
 src/components/layer.js                           |  2 +-
 src/components/material.js                        |  6 +++++-
 src/extras/primitives/primitives/a-sky.js         |  2 +-
 src/extras/primitives/primitives/a-videosphere.js |  2 +-
 src/utils/material.js                             | 23 +++++++++++------------
 6 files changed, 21 insertions(+), 17 deletions(-)

@vincentfretin

Copy link
Copy Markdown
Contributor

I verified with some console.log that by default on a texture we have

wrapS = THREE.ClampToEdgeWrapping;
wrapT = THREE.ClampToEdgeWrapping;
magFilter = THREE.LinearFilter;
minFilter = THREE.LinearMipmapLinearFilter;

so here the default on the minFilter and magFilter properties in this PR matches that and just setting minFilter to linear indeed does the same thing than the previous code.

Also I confirm this is still needed to set minFilter here.
I don't see any difference between minFilter LinearMipmapLinearFilter and LinearFilter on a 4096x2048 image on Quest 3
https://github.com/aframevr/aframe/blob/master/examples/boilerplate/panorama/puydesancy.jpg
but I see a difference in sharpness on a 8192x4096 image with a bit of flickering at times
https://github.com/c-frame/aframe-stereo-component/blob/master/examples/basic_image/textures/L.jpg
Of course it's even better with a mono equirect layer and there is no flickering at all.

So the changes in the PR looks good to me. Thanks for the work @mrxz
If some third party components used npot: true, it will need to be updated to use minFilter: linear instead.

@dmarcos

dmarcos commented May 12, 2025

Copy link
Copy Markdown
Member

Thanks!

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.

3 participants