godotengine / godot

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

Sprite3D AABB is always zero #49898

Open Zylann opened 3 years ago

Zylann commented 3 years ago

Godot version

3.3.2 stable

System information

Windows 10 64 bits GLES3

Issue description

VisualInstance.get_aabb() is supposed to provide bounds of any node that inherits from it. Sprite3D does have a size, but its get_aabb() function does not account for it, and in fact it returns an AABB with all dimensions and offset set to zero. I expected to obtain the same you would obtain with a MeshInstance having a QuadMesh of the same size for example (which returns -0.25, -0.25, 0 - 0.5, 0.5, 0)

Context: one of my plugins allows to instance scenes by dragging over colliders, and uses the AABB to estimate how much space they take. This causes distances to be largely under-estimated and too many scenes get painted.

Steps to reproduce

1) Create a Sprite3D 2) Assign a texture 3) Write a script that calls get_aabb() on the sprite3d and prints it 4) Run the game: observe the returned AABB is 0, 0, 0 - 0, 0, 0

Minimal reproduction project

Sprite3DAABB.zip

clayjohn commented 3 years ago

Is there any chance that you are calling get_aabb() before the sprite3D has properly initialized a size?

By default the AABB should be all zeroes, but it gets set when the texture of the Sprite3D is set. You can see how this works in the editor as the AABB is drawn as an orange box around the Sprite3D.

Zylann commented 3 years ago

I tried this in _ready with both MeshInstance and Sprite3D. MeshInstance returns a correct result, Sprite3D does not.

I tried in _process and now I see the AABB is returned non-zero the following frames... is that really required? It's documented nowhere (and hard to even document in get_aabb() because that makes implementation inconsistent). I kinda accepted this limitation for GUI due to complex layout rules, but here... it's just a sprite :|

kleonc commented 3 years ago

By default the AABB should be all zeroes, but it gets set when the texture of the Sprite3D is set.

@clayjohn The only place where AABB is being updated is in the _draw(). Actually updating it when setting the texture should indeed solve this issue. Edit: when texture is set the redraw is being enqueued so AABB isn't updated immediately.

clayjohn commented 3 years ago

We could look into setting the AABB immediately instead of queuing changes. I think that was more important when Sprites used ImmediateGeometry as updating was a very slow operation. But now that they use the Mesh API, updating and drawing is much faster.

insaneshri8 commented 3 years ago

Hlo i am new here . can anyone guide me please how can i contributes things need support . I want to contribute .

clayjohn commented 3 years ago

@insaneshri8 Right now when a property in Sprite3D is changed an update is "queued" meaning that the sprite itself waits until everything else is done before updating. This avoids having to regenerate the Sprite3D multiple times per frame. However, it also makes it so that the properties aren't updated until the next frame. One option we can explore is changing the _queue_update() function to _update() and have it immediately update the Sprite3D by calling _draw() directly instead of using call_deferred()

https://github.com/godotengine/godot/blob/7610409b8a14b8499763efa76578795c755a846d/scene/3d/sprite_3d.cpp#L184-L194

This needs extensive testing to measure the performance impact.

If it negatively impacts performance, we could consider splitting the _draw() function into two functions. The first function could update the Sprite3D properties (including AABB) and be called immediately on _update(). The second could just handle the slow parts (setting the material and sending mesh data to the GPU) and it could be called deferred.

insaneshri8 commented 3 years ago

@insaneshri8 Right now when a property in Sprite3D is changed an update is "queued" meaning that the sprite itself waits until everything else is done before updating. This avoids having to regenerate the Sprite3D multiple times per frame. However, it also makes it so that the properties aren't updated until the next frame. One option we can explore is changing the _queue_update() function to _update() and have it immediately update the Sprite3D by calling _draw() directly instead of using call_deferred()

https://github.com/godotengine/godot/blob/7610409b8a14b8499763efa76578795c755a846d/scene/3d/sprite_3d.cpp#L184-L194

This needs extensive testing to measure the performance impact.

If it negatively impacts performance, we could consider splitting the _draw() function into two functions. The first function could update the Sprite3D properties (including AABB) and be called immediately on _update(). The second could just handle the slow parts (setting the material and sending mesh data to the GPU) and it could be called deferred.

@insaneshri8 Right now when a property in Sprite3D is changed an update is "queued" meaning that the sprite itself waits until everything else is done before updating. This avoids having to regenerate the Sprite3D multiple times per frame. However, it also makes it so that the properties aren't updated until the next frame. One option we can explore is changing the _queue_update() function to _update() and have it immediately update the Sprite3D by calling _draw() directly instead of using call_deferred()

https://github.com/godotengine/godot/blob/7610409b8a14b8499763efa76578795c755a846d/scene/3d/sprite_3d.cpp#L184-L194

This needs extensive testing to measure the performance impact.

If it negatively impacts performance, we could consider splitting the _draw() function into two functions. The first function could update the Sprite3D properties (including AABB) and be called immediately on _update(). The second could just handle the slow parts (setting the material and sending mesh data to the GPU) and it could be called deferred.

this is my first issue ..and i dont know how to test it can u help? @clayjohn