Conversation
|
Please turn off Copilot reviews we do not accept them, thank you |
|
Thanks, turned it off. |
|
Just to confirm, did you use AI tools while making this PR? See here |
|
Yes. I read the contribution policy in full before submitting the PR. The actual changes were made by me. I only used AI at the end as an additional quality check, and treated its suggestions as reference rather than something to apply blindly. |
|
Great thank you! |
|
So, this is a substantial step forward. I built this, everything runs without errors, and I also got all your test cases to pass. This fixes one of the main issues mentioned in #119462. The one case that I am getting to break is with this example: For whatever reason, pixel art textures used on the TextureRect or on the Sprite2D both use Linear instead of Nearest. I'm pretty sure this is not intentional? If Viewport specifies a Edit above. |
Co-authored-by: Jack Moody <74024384+radioactivejackal1414@users.noreply.github.com>
|
I tried reproducing your example and noticed that either setting the SubViewportContainer’s Texture Filter to Nearest or changing the project’s default filter to Nearest made the result sharp. I think there may be two filtering stages here: the SubViewport renders the Sprite2D and TextureRect using Nearest, then the SubViewportContainer displays that output texture using its own filter. In this setup, the container inherits Linear, which could explain the blur when magnified. If that’s the case, this might be expected behavior rather than an inheritance issue. |
| </member> | ||
| <member name="canvas_item_default_texture_filter" type="int" setter="set_default_canvas_item_texture_filter" getter="get_default_canvas_item_texture_filter" enum="Viewport.DefaultCanvasItemTextureFilter" default="1"> | ||
| The default filter mode used by [CanvasItem] nodes in this viewport. | ||
| The root viewport's default filter is set by [member ProjectSettings.rendering/textures/canvas_textures/default_texture_filter]. To inherit filtering from the parent node or viewport in another viewport, use [constant DEFAULT_CANVAS_ITEM_TEXTURE_FILTER_PARENT_NODE]. |
There was a problem hiding this comment.
"viewport in another viewport" isn't very clear here, what does it mean?
There was a problem hiding this comment.
I meant a Viewport nested under another Viewport in the scene tree.
I wanted to clarify how inheritance works in that case but I agree the wording is confusing.Since the enum description already explains this, I'll remove it.
Thanks for pointing this out, I missed that PR. It looks like we addressed the same fallback issue. This PR also handles cases where a plain Node sits between Viewports, makes sure filter changes reach descendant nodes, and adds regression tests. |
I wasn't exactly sure, but I think your explanation makes sense. All the user has to do is set the filter they want on the SubViewportContainer which is easy to do (albeit possibly confusing for someone not familiar with the filtering inheritance system). If it is indeed an issue then it could be fixed in a follow up. |
|
It would depend on what is the intended behavior and expected behavior indeed, if the current behavior is intentional or expected changing it would break compatibility, so that'd have to be considered |
What problem(s) does this PR solve?
CanvasItemresolves to the viewport default, or when an ordinaryNodesits between it and the enclosing Viewport.Additional information
This changes the inheritance lookup so it can fall back to the nearest enclosing Viewport when needed, while still respecting explicit
CanvasItemfilters and explicit Viewport filters.Filter-change propagation now also crosses intermediate nodes.
The default texture filter of
WindowandSubViewportis unchanged; this only affects Viewports configured to inherit their filter.I also added regression tests for nested Viewports, intermediate nodes, explicit overrides, reparenting, and Window/Popup cases. The existing Viewport tests pass with the fix.