Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PackedScene.instance() should not cause lag or a different way to instance interactively should be provided #1801

Open
winston-yallow opened this issue Nov 10, 2020 · 25 comments

Comments

@winston-yallow
Copy link

Describe the project you are working on:
We are working on a small open world game. The world is mostly auto generated but large, and the player can move around freely.

Describe the problem or limitation you are having in your project:
The world grew to a size where it began to lag so we implemented chunk loading/unloading. This works smoothly, but every time we call .instance() we get a small lag of about 40ms to 60ms. A reproduction project can be found in godotengine/godot#43439

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

Adding a non blocking way to instance scenes would allow us to load chunks more smoothly.

From a user perspective I see two possible solutions:

  1. When generating a PackedScene (for example with ResourceInteractiveLoader) everything should already be loaded so far that instancing is fast. This allows the user to preload the scene smoothly with ResourceInteractiveLoader as currently advertised for background loading.
  2. PackedScene could provide a method instance_interactive() which works in a similar way to ResourceLoader.load_interactive(). This allows the user to poll each frame to check if the loading has finished yet.

I think Option 2 would be easier to implement as it does not require a change of what PackedScene stores internally.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

I can only describe this from the user perspective. Code using approach number 2 could look like this:

var scene: PackedScene = preload('Scene.tscn')
var instancer: InteractiveInstancer

func _ready() -> void:
    instancer = scene.instance_interactive()

func _process(_delta: float) -> void:
    match instancer.poll():
        InteractiveInstancer.FINISHED:
            var node := instancer.get_node()
            add_child(node)
            set_process(false)
        InteractiveInstancer.OK:
            pass # Continue instancing
        InteractiveInstancer.ERROR:
            push_error(instancer.get_error())
            set_process(false)

If this enhancement will not be used often, can it be worked around with a few lines of script?:
It would require more than just a few lines. One could use PackedScene._bundled to create the nodes one by one.
I am not sure if this should be done as _bundled seems more like an implementation detail that should not be relied on.

Is there a reason why this should be core and not an add-on in the asset library?:
Scene instancing is a core feature of godot that should (in my personal opinion) provide a way of loading & instancing scenes seamlessly.

@Zireael07
Copy link

Is the thing you're instancing something a player can see? A Mesh, a Sprite...? If yes, the lag you're seeing is likely shaders compiling (especially as you said it only happens the first time)

@winston-yallow
Copy link
Author

Yes, these are meshes with Materials. However I just tried using the default material with the same result while having a cube in the main scene to ensure the default material is already compiled.

@Calinou Calinou changed the title PackedScene.instance() should not cause lag or a different way to instance interactively should be provided PackedScene.instance() should not cause lag or a different way to instance interactively should be provided Nov 10, 2020
@mrjustaguy
Copy link

I think this is an OpenGL issue you're seeing here.. afaik Vulkan shouldn't have the issue.. Try running it on a Nightly build you can find at https://hugo.pro/projects/godot-builds/ and see how the behavior changes.

@winston-yallow
Copy link
Author

winston-yallow commented Nov 11, 2020

I can't even create a script or setup the scene in 4.0 builds before they crash for me (for unrelated reasons I think).
Will try once they are more stable. It also does not solve the problem for 3.2, or are you suggesting that it is not possible to fix in OpenGL? That would be kinda bad for us as we really need to load chunks smoothly.

@mrjustaguy
Copy link

mrjustaguy commented Nov 11, 2020

Yea I think it's an OpenGL issue that is probably not fixable.. Maybe there is some workaround but don't count on it.. Then again I could be wrong...

Edit: I mean If it is a 1 time lag spike when instancing and if it is due to shader compilation which OpenGL can't handle, look at https://en.wikipedia.org/wiki/Vulkan_(API) under features the 6th point... Vulkan allows shader Pre-compilation according to that.

@Calinou
Copy link
Member

Calinou commented Nov 11, 2020

@mrjustaguy Judging by this comment, the issue here isn't related to OpenGL shader compilation:

Yes, these are meshes with Materials. However I just tried using the default material with the same result while having a cube in the main scene to ensure the default material is already compiled.

@mrjustaguy
Copy link

You might be right @Calinou as upon further testing.. I've found A solution to your problem @winston-yallow and it's fairly simple.. Initialize var scene up right after the preload and make the 1st instance at startup without adding a child.. here's the modified project:
Fix.zip

Note: I Commented out the prints and used the profiler, as i noticed a 300ms slowdown due to the prints, however when uncommented the prints indicate about a 1-2ms instance time

@winston-yallow
Copy link
Author

I would like to not instance the scene right at the beginning. As stated the purpose is a chunk loader. We can not instance every chunk of the world once at the start... That would be the equivalent of loading the whole world. Which is exactly what we are trying not to do. As for the print statements: Yes, they do have a performance penalty (however on my computer it is much less than 300ms) but the lag is noticeable even without print statements.

@mrjustaguy
Copy link

mrjustaguy commented Nov 12, 2020

Well, only other idea I have would be to load in a thread to prevent the main thread from lagging out when first instancing...
see: https://docs.godotengine.org/en/stable/tutorials/threads/thread_safe_apis.html and https://docs.godotengine.org/en/stable/tutorials/threads/using_multiple_threads.html
Also, You -Are- Loading the whole world with the preloads.. at the very start of the game, so I don't really see why instancing the whole world once upon initialization would be such a big deal, it's not like you need to add them all to the world with add_child() as the issue is the 1st initalization lagging, subsequent ones don't

@winston-yallow
Copy link
Author

@mrjustaguy I only use preload in the example to simplify things and show the effect of instancing. In my project I do use a thread to load stuff asynchronously. But instancing still has this effect. As you can see in the original issue I ran into godotengine/godot#36793 when doing it in a thread. So no, this is not a solution to this.

@mthreis
Copy link

mthreis commented Jan 1, 2021

To me, that feature is a must not only for chunk loading but also scene startup. There are many things you'd want to do dynamically at the scene startup but it's not so trivial due to that. In two of my games I have issues with this.

In one, I have to create colliders (and a bunch of other objects) for a scene based on the "shape" of the map and some other global info but can't do it all async (with C#, I can both load and instance things async, but can't add_child). It's a shame that the only solution I have for that is to create some sort of Factory object that will spawn few things synchronously every frame. In this case, you can still get hiccups in a slow machine and the loading times will be long even on fast PCs because you basically have to set a magic number for the amount of objects you can spawn each frame.

In another, I have to instance chunks and there's no workaround for this because it's a whole hierarchy of objects that I spawn at once, not many objects. There's no way around this and the lag spikes can be felt in an old laptop (decided to postpone this project until there's a way to do it properly).

@winston-yallow
Copy link
Author

I feel like for the first part (scene startup) this is not really relevant, that is exactly what one would normally do in a scene manager. I do this in a project too, basically my scene loader creates the whole scene in a thread and then synchronizes with the main thread to add it as the current scene. That is kinda okay in my opinion, the small lag when synchronizing and instancing is okay as it does not happen during gameplay.

The second part really is the annoying part. It does not affect me personally anymore (we decided to move that project over to unreal due to this), but I would still like to see this happen for future projects. I feel like there should be a way to not have a lagging .instance() method.

As a side-note: Godot 4.0 finally runs without an immediate crash on my machine, and the instancing seems to work a bit smoother. I will need to test this a bit more though. This could hint to this being OpenGL related, although ensuring the shaders are loaded did not make a difference (as noted in this comment)

@smks
Copy link

smks commented Mar 27, 2022

Is this problem going to be a 'fixed in Godot 4.0'? As I too am experiencing this and it is quite a jarring experience.

@Calinou
Copy link
Member

Calinou commented Mar 27, 2022

Is this problem going to be a 'fixed in Godot 4.0'? As I too am experiencing this and it is quite a jarring experience.

Threaded loading was added in master, but threaded instancing is most likely not technically feasible as the scene tree isn't thread-safe. Feel free to test this in 4.0alpha anyway. See also godotengine/godot#48978.

@DasOhmoff
Copy link

DasOhmoff commented May 9, 2022

Is there any news in regards to this. I have the same issue on mobile. How can I avoid the lag when using instance for the first time? Is there some work around?

@mrjustaguy
Copy link

using it for the first time means it's probably loading up the resource, so loading on a thread (before you need to instance it) should fix this issue, although I haven't tested that, so don't know for sure.

@winston-yallow
Copy link
Author

@mrjustaguy this is the second time you suggest using a thread. It does not solve the problem of instancing. It certainly can help a bit but wont solve it 100%

@DasOhmoff depending on what is causing the lag you might want to ensure that all materials are loaded and compiled before. I think there is a plugin that can load all meterials once so that opengl compiles them. It won't solve the problem from this github issue, but it will speed it up

@DasOhmoff
Copy link

@mrjustaguy, @winston-yallow Ah I see. Thank you.

I looked through some news and found this: godotengine/godot#53411, do you think this might help? If I understood correctly from your previous comments, the stutters occur even without shader compilation, correct?

@mrjustaguy
Copy link

You're right. but like the only reasons I see that aren't just clearly a bug and are just known restrictions of how the engine works that apply only for first time instancing are:

  1. Having to Wait for Loading the Scene file (.tscn and .scn)
  2. Having to Wait for Loading the assets for the Scene (meshes, textures, scripts, materials....)
  3. Having to Compile Shaders for the assets for the Scene

There is simply nothing else that I'm aware of in the pipeline that would affect ONLY the first instance of a scene.

@mthreis
Copy link

mthreis commented May 11, 2022

That's still something I look forward to! There's this tweet of reduzio: https://twitter.com/reduzio/status/1455312934549299206. For those who don't want to click the link, it says:

"Godot 3.x was very difficult to optimize due to the ancient architecture, but this is not the case with 4.x.

The biggest item will be making the scene system threadable, and implement an easy to use job system for node processing..".

I'm assuming the "making the scene system threadable" part is what we're all looking for, but I couldn't find anything on github. Thoughts?

@mrjustaguy
Copy link

One thing to consider with this, it is quite possible that this is actually a PCIe Bottleneck.. I've found that with sufficiently gigantic assets, when they're instanced for the first time, the loading goes smoothly, but stutters when it's moving from RAM to VRAM, and which also explains why further instances don't cause the stutters, only the 1st one.

Similar result can be gotten by just using ResoruceLoader to load an asset on a thread, which will automatically move any Video Assets into VRAM once it is done loading into RAM (causing the stutter at that point)
Instancing the Asset at that point has no stuttering, as if it were instanced once.

hence the stutter from first time instancing, noise generation and the like are actually caused by PCIe Bottleneck, as it just overwhelms the PCIe bus with the transfer, preventing normal CPU<->GPU communication required for telling the GPU what to render which means the GPU is left idle during transfer, even if the GPU Doesn't need any of the data from the offending resources.

The only solution to this that I see is finding a way to limit how much bandwidth is used for moving assets to VRAM

@Calinou
Copy link
Member

Calinou commented May 17, 2022

The only solution to this that I see is finding a way to limit how much bandwidth is used for moving assets to VRAM

Asset streaming is required for this (for both textures and models).

@xiaoliang6669
Copy link

I also encountered the problem of "tilemap" resources being loaded and unloaded dynamically, which caused the problem of "stuck"

@Calinou
Copy link
Member

Calinou commented Dec 2, 2022

@xiaoliang6669 Please keep the discussion about your specific issue in godotengine/godot#69437.

@TheNetherPug
Copy link

TheNetherPug commented Feb 17, 2024

I know this is severe necroposting, but I've found myself encountering a lag with PackedScene.instantiate() in Godot 4.2 as well. Is there any news in regards to this proposal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants