godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Change the error for creating ViewportTexture for not 'local to scene' resources into a warning #8578

Open ilyabelow opened 9 months ago

ilyabelow commented 9 months ago

Describe the project you are working on

Any project involving a ViewportTexture drawn as a texture on a mesh

Describe the problem or limitation you are having in your project

It's a minor annoyance, but it still bug me every time: when I try to set a ViewportTexture in a material, I see

Can't create a ViewportTexture on this resource because it's not set as local to scene.
Please switch on the 'local to scene' property on it (and all resources containing it up to a node).

without a proper explanation why. I have to set mesh and material as "local to scene", then return to shader parameters and set my texture

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I propose changing the error to a skippable warning that reads something like "Parent resources are not local to scene, so ViewportTexture won't work! Please fix this". By skippable I mean that you can press "Ok" and still set the texture - and fix parent resources afterwards

Why do I think the warning is better than an error?

  1. Proposed approach doesn't break user's flow - they want to set the texture and they will not be interrupted
  2. The error is a lie: you CAN in fact set a ViewportTexture, it will just not work (because physical texture initialization is implemented in ViewportTexture::setup_local_to_scene) and it will make no sense as soon as the scene is instantiated more than once (which is not so obvious to be honest)
  3. It will be more consistent: now I can "cheat" this error ans if I set a ViewportTexture and then disable "local to scene" checkbox on parent resources, no error or warning will be displayed (as it is impossible to implement) as if it is ok. This behavior breaks the notion of impossibility of setting a ViewportTexture, and a warning about it not working would be more appropriate - it's your fault you unchecked locality to scene even though we warned you it won't work

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

If there is a skippable warning dialog, it's trivial to implement. I haven't found one yet, so it's possible to just change the wording of the error to be less strict, and maybe make it show once per texture

If this enhancement will not be used often, can it be worked around with a few lines of script?

It's a part of the engine

Is there a reason why this should be core and not an add-on in the asset library?

Same reason

AThousandShips commented 9 months ago

I'd argue that anything that doesn't work should have an error, not a warning, warnings are easy to miss or ignore

The details on the error can be improved to be more clear, but I don't think it should be a warning

ilyabelow commented 9 months ago

I'd argue that anything that doesn't work should have an error, not a warning, warnings are easy to miss or ignore

Fair. But partially my point still holds: the error text is indeed unclear, and I think "this will not work, don't do this, fix locality to scene first" is better than abstract restriction "you can't do this"

AThousandShips commented 9 months ago

That's what I said?

The details on the error can be improved to be more clear, but I don't think it should be a warning

ilyabelow commented 9 months ago

That's what I said?

Yeah, yeah, I've just elaborated, sorry it sounded weird :)

Also, the error may be more verbose like "this will not work as every instance of the scene will have its own viewport which will output different texture"