fix(texture): fix texture overblur with st-blur parameters#5071
Conversation
c2a8f95 to
51c2b9b
Compare
|
@lgritz, I think the PR is in good shape now and ready for review. Last changes provided:
|
|
@lecocqp Can you rebase on top of the current main, I think I have fixed a number of pesky intermittent CI failures that are unrelated to your changes. |
src/libtexture/imagecache.cpp
Outdated
| } else if (name == "legacy_texture_blur" && type == TypeDesc::INT) { | ||
| m_legacy_texture_blur = (*(const int*)val != 0); |
There was a problem hiding this comment.
I suspect that the new field and handling of it should be in the TextureSystem and not in the ImageCache?
There is a division of labor here, and things specific to texture filtering go in the TextureSystem, while things related the file reads and tile caching policy are part of the ImageCache. Requests to the TS to set attributes in the underlying IC are just passed down transparently, so that from the renderer level, you can just set texturesys->attribute(...) and not need to know what lives where.
lgritz
left a comment
There was a problem hiding this comment.
Other than my comment about in which class the new field should live, it LGTM
11afd1b to
93e1e4b
Compare
|
I moved the new I also rebased my branch on top of main, but I'm still seeing some CI failures. |
Random CI failures have really been kicking my ass all week. These don't seem to be related to your PR. |
lgritz
left a comment
There was a problem hiding this comment.
LGTM!
I'll merge as soon as CI is finished. (And pending your acceptance of my #if suggestion.)
|
Holy shit, why does every edit seem to fail MORE tests in CI, and none of them are related to this PR as near as I can tell? What is happening to the GitHub runners? |
This PR fixes the texture blur overstimate reported in AcademySoftwareFoundation#5069 We provide the following changes: * Use the mathematically correct Pythagorean form to properly adjust the ellispse extents. * Add a new legacy_texture_blur attribute in TextureSystem to opt into the fix without breaking existing renders. * Add a new --fix-texture-blur option in testtex to enable the fix. * Add a new texture-blurfix test. Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
…xture_blur` and default it to false * Rename testtex option to `--legacy-texture-blur` and default it to false. * Modify texture-blurfix test to check the fixed correct blur and the old legacy blur. Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
Co-authored-by: Larry Gritz <lg@larrygritz.com> Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
7d4194d to
3b254ae
Compare
|
I rebased this again, in the hopes that on top of the other fixes I merged, we'll finally pass CI. If it does, I will merge it as well. |
…ftwareFoundation#5071) Fixes AcademySoftwareFoundation#5069 This PR fixes the texture blur overstimate reported in AcademySoftwareFoundation#5069 We provide the following changes: * Use the mathematically correct Pythagorean form to properly adjust the ellispse footprint with st-blur parameters. * Add a new legacy_texture_blur attribute in TextureSystem to opt into the fix without breaking existing renders. * Add a new --fix-texture-blur option in testtex to enable the fix. * Add a new texture-blurfix test. Note: This PR has been partially edited using the Claude coding assistant. --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
…ftwareFoundation#5071) Fixes AcademySoftwareFoundation#5069 This PR fixes the texture blur overstimate reported in AcademySoftwareFoundation#5069 We provide the following changes: * Use the mathematically correct Pythagorean form to properly adjust the ellispse footprint with st-blur parameters. * Add a new legacy_texture_blur attribute in TextureSystem to opt into the fix without breaking existing renders. * Add a new --fix-texture-blur option in testtex to enable the fix. * Add a new texture-blurfix test. Note: This PR has been partially edited using the Claude coding assistant. --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
The m_legacy_texture_blur flag is set in TextureSystemImpl::Init, which overrides the default initialisation depending on the OIIO version. In other words, the texture blur fix was always enabled, regardless of the OIIO version. I also updated the help description for this flag in testtex. This fixes minor shortcomings of #5071 --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
…ndation#5080) The m_legacy_texture_blur flag is set in TextureSystemImpl::Init, which overrides the default initialisation depending on the OIIO version. In other words, the texture blur fix was always enabled, regardless of the OIIO version. I also updated the help description for this flag in testtex. This fixes minor shortcomings of AcademySoftwareFoundation#5071 --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
…ndation#5080) The m_legacy_texture_blur flag is set in TextureSystemImpl::Init, which overrides the default initialisation depending on the OIIO version. In other words, the texture blur fix was always enabled, regardless of the OIIO version. I also updated the help description for this flag in testtex. This fixes minor shortcomings of AcademySoftwareFoundation#5071 --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com>
…ftwareFoundation#5071) Fixes AcademySoftwareFoundation#5069 This PR fixes the texture blur overstimate reported in AcademySoftwareFoundation#5069 We provide the following changes: * Use the mathematically correct Pythagorean form to properly adjust the ellispse footprint with st-blur parameters. * Add a new legacy_texture_blur attribute in TextureSystem to opt into the fix without breaking existing renders. * Add a new --fix-texture-blur option in testtex to enable the fix. * Add a new texture-blurfix test. Note: This PR has been partially edited using the Claude coding assistant. --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com> Signed-off-by: Vlad (Kuzmin) Erium <libalias@gmail.com>
…ndation#5080) The m_legacy_texture_blur flag is set in TextureSystemImpl::Init, which overrides the default initialisation depending on the OIIO version. In other words, the texture blur fix was always enabled, regardless of the OIIO version. I also updated the help description for this flag in testtex. This fixes minor shortcomings of AcademySoftwareFoundation#5071 --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com> Signed-off-by: Vlad (Kuzmin) Erium <libalias@gmail.com>
…ftwareFoundation#5071) Fixes AcademySoftwareFoundation#5069 This PR fixes the texture blur overstimate reported in AcademySoftwareFoundation#5069 We provide the following changes: * Use the mathematically correct Pythagorean form to properly adjust the ellispse footprint with st-blur parameters. * Add a new legacy_texture_blur attribute in TextureSystem to opt into the fix without breaking existing renders. * Add a new --fix-texture-blur option in testtex to enable the fix. * Add a new texture-blurfix test. Note: This PR has been partially edited using the Claude coding assistant. --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com> Signed-off-by: Vlad (Kuzmin) Erium <libalias@gmail.com> Signed-off-by: Vlad <shaamaan@gmail.com>
…ndation#5080) The m_legacy_texture_blur flag is set in TextureSystemImpl::Init, which overrides the default initialisation depending on the OIIO version. In other words, the texture blur fix was always enabled, regardless of the OIIO version. I also updated the help description for this flag in testtex. This fixes minor shortcomings of AcademySoftwareFoundation#5071 --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com> Signed-off-by: Vlad (Kuzmin) Erium <libalias@gmail.com> Signed-off-by: Vlad <shaamaan@gmail.com>
…ftwareFoundation#5071) Fixes AcademySoftwareFoundation#5069 This PR fixes the texture blur overstimate reported in AcademySoftwareFoundation#5069 We provide the following changes: * Use the mathematically correct Pythagorean form to properly adjust the ellispse footprint with st-blur parameters. * Add a new legacy_texture_blur attribute in TextureSystem to opt into the fix without breaking existing renders. * Add a new --fix-texture-blur option in testtex to enable the fix. * Add a new texture-blurfix test. Note: This PR has been partially edited using the Claude coding assistant. --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com> Signed-off-by: Vlad <shaamaan@gmail.com>
…ndation#5080) The m_legacy_texture_blur flag is set in TextureSystemImpl::Init, which overrides the default initialisation depending on the OIIO version. In other words, the texture blur fix was always enabled, regardless of the OIIO version. I also updated the help description for this flag in testtex. This fixes minor shortcomings of AcademySoftwareFoundation#5071 --------- Signed-off-by: Pascal Lecocq <pascal.lecocq@gmail.com> Signed-off-by: Vlad <shaamaan@gmail.com>
Fixes #5069
This PR fixes the texture blur overstimate reported in #5069
We provide the following changes:
Note: This PR has been partially edited using the Claude coding assistant.
Checklist:
behavior.
testsuite.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.