Fix wrong indices used for transform & UBO matrix for double precision build#111403
Conversation
AeioMuch
force-pushed
the
fix_double_precision_wrong_indexes
branch
from
033114f to
1c92c30
Compare
AThousandShips
changed the title
clayjohn
left a comment
clayjohn
left a comment
There was a problem hiding this comment.
The changes to the indices look great! Nice work.
I left comments on the other changes that were added. In general we prefer if bug fix PRs are restricted to fixing the particular bug and don't include unrelated stylistic changes. So please go ahead and remove the changes that aren't related to the fix
servers/rendering/renderer_rd/storage_rd/render_scene_data_rd.cpp
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.cpp
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.cpp
Outdated
Show resolved
Hide resolved
AeioMuch
force-pushed
the
fix_double_precision_wrong_indexes
branch
from
c1255f3 to
6889825
Compare
AeioMuch
force-pushed
the
fix_double_precision_wrong_indexes
branch
from
6889825 to
dae2122
Compare
|
Yes sorry, I saw only one or two warning(s) by clangd per each of those files; so I figured, let me sneak those changes in at the same time since it's so little, I though why not, no idea if it's useful apart from the removal of the warning 😅 But I 100% understand it's not ideal to include those unrelated changes here, lol. Anyway, removed ! |
Thanks! I appreciate your quick response. To be clear, its not like we don't want to fix things that are creating warnings. But I don't want to block merging this clear fix while we discuss the performance tradeoffs of zeroing a struct right before filling that struct in order to fix a warning that isn't actually harming anything. Unfortunately in hot paths we need to be super careful as even the cost of writing 0 to a bunch of properties can add up. |
Oh yes make sense, thanks for the explanation and for pointing it out! |
|
Thanks! |
4 participants
Fix #111308