godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.51k stars 21.26k forks source link

Make preload invalid paths null instead of a compilation error #7118

Closed HeartoLazor closed 7 years ago

HeartoLazor commented 8 years ago

Operating system or device - Godot version: Windows 10 Godot 2.1 stable

Issue description: The current behavior of preload with invalid paths is give an error in editor (Can't preload resource at path) and crash at runtime. xh5vlyr 1 A better approach could be instead of give an error in editor and crash at runtime, preload could return null and let the user check ingame if the preloaded var/const is null.

This could be useful for addons, for example I have an addon which has a timer created by script for delayed triggers and I wan't the trigger addon to be independant from the project, but for this project the timer could be paused with a player ability, so I need a way to attach that script for the timer in the trigger. The easiest and optimal way to achieve this could be preload const and check if the script path is null. So with this I could use the addon without major modifications in another project or I could put another timer script at the path for custom behaviour in a new project.

NOTE: I'm aware of load and check/load the path in ready:

const _timer_script_path = "res://resources/scripts/timer.gd"

func _ready():
    var timer = Timer.new()
    var script = load(_timer_script_path)
    if(script != null):
        timer .set_script(script)

But that method should be less optimal because is 1 load for EACH ready vs a SINGLE preload in a CONST at start of the game

Steps to reproduce: Create a script and try to preload an invalid resource with a const and run the game: const _script = preload("res://resources/scripts/timer.gd")

Zylann commented 8 years ago

I agree a crash is not great. Your proposal should be fine if it doesn't implies a runtime cost (besides initial load).

reduz commented 8 years ago

Crash (parse error actually) is fine, this is meant to always work. It would be worse if you mess up the name, it doesn t work and you dont realize you made a mistake.

On Mon, Nov 14, 2016 at 5:49 PM, Marc notifications@github.com wrote:

I agree a crash is not great. Your proposal should be fine if it doesn't implies a runtime cost (besides initial load).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/7118#issuecomment-260458165, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z28rXhNi8w45Eh-9cs7y4uSKDqr2Lks5q-MlZgaJpZM4Kxtpp .

reduz commented 8 years ago

If you are not sure whether it should work or not, use load() instead.

On Mon, Nov 14, 2016 at 6:29 PM, Juan Linietsky reduzio@gmail.com wrote:

Crash (parse error actually) is fine, this is meant to always work. It would be worse if you mess up the name, it doesn t work and you dont realize you made a mistake.

On Mon, Nov 14, 2016 at 5:49 PM, Marc notifications@github.com wrote:

I agree a crash is not great. Your proposal should be fine if it doesn't implies a runtime cost (besides initial load).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/7118#issuecomment-260458165, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z28rXhNi8w45Eh-9cs7y4uSKDqr2Lks5q-MlZgaJpZM4Kxtpp .

HeartoLazor commented 8 years ago

Crash (parse error actually) is fine, this is meant to always work.

The app should still crash if the user doesn't handle the null preloaded var/const with the approach in my post.

It would be worse if you mess up the name, it doesn t work and you dont realize you made a mistake.

With load the user could mess up the name and nobody complained about that, this is the same behavior suggested for preload in this issue. The advantage of nullable preloads is custom behaviour for preload paths not found instead of "I can't compile with an invalid path".

If you are not sure whether it should work or not, use load() instead.

Well as I said in first post I'm aware of load, but that method should be less optimal because is 1 load for EACH ready vs a SINGLE preload in a CONST at start of the game.

reduz commented 8 years ago

you can run load in an initializer fine:

var someobj = load(pathtores)

On Mon, Nov 14, 2016 at 7:36 PM, Hearto notifications@github.com wrote:

Crash (parse error actually) is fine, this is meant to always work.

The app should still crash if the user doesn't handle the null preloaded var/const with the approach in my post.

It would be worse if you mess up the name, it doesn t work and you dont realize you made a mistake.

With load the user could mess up the name and nobody complained about that, this is the same behavior suggested for preload in this issue. The advantage of nullable preloads is custom behaviour for preload paths not found instead of I can't compile with an invalid path.

If you are not sure whether it should work or not, use load() instead.

Well as I said in first post I'm aware of load, but that method should be less optimal because is 1 load for EACH ready vs a SINGLE preload in a CONST at start of the game.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/7118#issuecomment-260485937, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z20rIpdpIMFF5KFK3isQocq_Q0iuZks5q-OJcgaJpZM4Kxtpp .

Sslaxx commented 8 years ago

6203 mentions some of the things I ran into with load/preload, might be worth bearing in mind?

punto- commented 8 years ago

Preload is supposed to be a static dependency, if it doesn't exist, it's the same as a .tscn mentioning an external resource that doesn't exist.. It should abort as if the game data is incorrect

On 14 November 2016 at 18:30, Juan Linietsky notifications@github.com wrote:

If you are not sure whether it should work or not, use load() instead.

On Mon, Nov 14, 2016 at 6:29 PM, Juan Linietsky reduzio@gmail.com wrote:

Crash (parse error actually) is fine, this is meant to always work. It would be worse if you mess up the name, it doesn t work and you dont realize you made a mistake.

On Mon, Nov 14, 2016 at 5:49 PM, Marc notifications@github.com wrote:

I agree a crash is not great. Your proposal should be fine if it doesn't implies a runtime cost (besides initial load).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/7118# issuecomment-260458165, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z28rXhNi8w45Eh- 9cs7y4uSKDqr2Lks5q-MlZgaJpZM4Kxtpp .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/7118#issuecomment-260468804, or mute the thread https://github.com/notifications/unsubscribe-auth/AGVmPQfSEFpuuexYxIjcPtFkdtCTlESjks5q-NMHgaJpZM4Kxtpp .

HeartoLazor commented 8 years ago

you can run load in an initializer fine:

var someobj = load(pathtores)

What about CONST values? At least you can't load in a CONST obj

reduz commented 8 years ago

what does it change for you whether it s a const value or not? the only difference it makes to gdscript is that it can do compile time optimization.

On Mon, Nov 14, 2016 at 7:54 PM, Hearto notifications@github.com wrote:

you can run load in an initializer fine:

var someobj = load(pathtores)

What about CONST values? At least you can't load in a CONST obj

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/7118#issuecomment-260490538, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z27RAUX1T4KNMXqNqBN9LNlbP-KeMks5q-Oa-gaJpZM4Kxtpp .

vnen commented 8 years ago

AFAIK resources are cached, so a second load() will grab the same resource from memory, not load from the filesystem again.

Sslaxx commented 8 years ago

If it does, the resource doesn't have the same ID.

akien-mga commented 8 years ago

preload expects a const String. If that const String points to an invalid path, that's a bug from whoever wrote the string, and it's normal that the program gives an error.

You can't use preload to load resources dynamically, that's load's job, so it's definitely what you want to use in your use case. It makes no sense to have preload return a null object if passed a bogus path. It should only return a null object if it was called on a valid path but an invalid resource (i.e. it couldn't load the file because it's not a resource Godot knows how to handle), not if the programmer made a typo.

akien-mga commented 8 years ago

If it does, the resource doesn't have the same ID.

How is that a problem? As long as each ID points to the same allocated memory, that's AFAIK the way it's intended to be. Resources are reference-counted - the allocated memory will live until all references are freed.

neikeq commented 8 years ago

Agree with Akien. Also I prefer a compile error than a successful compilation but a hidden bug.

HeartoLazor commented 8 years ago

what does it change for you whether it s a const value or not? the only difference it makes to gdscript is that it can do compile time optimization.

Well in theory with CONST the value should be executed 1 time for each script and with var the value should be executed for each instance of the script. Now that sound less optimal but taking in account this reply from @vnen :

AFAIK resources are cached, so a second load() will grab the same resource from memory, not load from the filesystem again.

With that info var xxx = load() should be the same performance wise as const xxx = preload(). Of course if the cache algorithm isn't limited (something like using a dictionary with a limit of 100 last values) So this issue could be closed if the performance is the same in both cases.

bojidar-bg commented 8 years ago

Well in theory with CONST the value should be executed 1 time for each script and with var the value should be executed for each instance of the script.

That's true, but if it really, really matters, you can make a global singleton holding those variables and doing whatever you want with the resources just once.

mhilbrunner commented 7 years ago

@akien-mga Close this?