Make lights_per_object configurable #43606
Conversation
|
Looks great! Before your PR is ready to go, you will have to squash your 3 commits together as described here https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase |
ok done :) |
NHodgesVFX
force-pushed
the
3.2
branch
2 times, most recently
from
063f0bf
to
c7d38b4
Compare
NHodgesVFX
force-pushed
the
3.2
branch
3 times, most recently
from
f9ea198
to
d42f7e1
Compare
| @@ -1834,15 +1834,14 @@ void RasterizerSceneGLES3::_render_geometry(RenderList::Element *e) { | |||
|
|
|||
| void RasterizerSceneGLES3::_setup_light(RenderList::Element *e, const Transform &p_view_transform) { | |||
|
|
|||
| int omni_indices[16]; | |||
| int maxobj = state.max_forward_lights_per_object; | |||
| int *omni_indices = (int *)alloca(maxobj * sizeof(int)); | |||
There was a problem hiding this comment.
MSVC doesn't seem to like alloca here:
D:\a\godot\godot\drivers\gles3\rasterizer_scene_gles3.cpp(2219) : error C2220: the following warning is treated as an error
D:\a\godot\godot\drivers\gles3\rasterizer_scene_gles3.cpp(2219) : warning C4750: 'void __cdecl RasterizerSceneGLES3::_setup_light(struct RasterizerSceneGLES3::RenderList::Element * __ptr64,class Transform const & __ptr64) __ptr64': function with _alloca() inlined into a loop
Seems like you might have to remove _FORCE_INLINE_ for it to compile.
BTW, would it make sense to use something smaller than int to allocate less memory (e.g. uint8_t or uint16_t)? Or it doesn't matter?
There was a problem hiding this comment.
it would use less memory but even with 1000 lights it only uses around 0.005mb, so I don't really think it matters either way
There was a problem hiding this comment.
int is probably fine here as the max size is 1024 * sizeof (int), but in general when using the stack, akien is absolutely right, because the stack can be of very limited size, platform dependent, and how much room you have 'depends'. It absolutely is not the same as general memory.
See e.g. here, suggesting historical figures of 8K being possible on old mobile:
https://stackoverflow.com/questions/1859327/android-stack-size
So 1024 * 8 byte int = 8K, blown stack. Whereas 1024 * 2 byte uint16_t = slightly safer, but still likely to blow such a small stack.
More realistically, modern times I would expect around a 1 meg+, especially on desktop:
https://pspdfkit.com/blog/2019/android-large-memory-requirements/
But this is not just for your function.
In short, alloca is great but make sure you know what you are doing, and beware of anything that might be called recursively. This kind of thing is why the compiler refuses to compile with a FORCE_INLINE.
|
Fantastic! Thank you! |
|
Thanks! |
you can now have lots of lights, thanks to jonbonazza for helping me with this pr
some videos of it:
https://streamable.com/mjb9gw
https://streamable.com/npu07o