Fix aliasing issue in framework #300
Conversation
|
|
Lamakaio
force-pushed
the
aliasing_fix
branch
from
732a7fa to
ddff6d7
Compare
There was a problem hiding this comment.
The old code with the funny comment worked because it worked with an unsigned char array first, then used it to initialize a struct afterwards.
This memcpy approach looks a little cleaner.
About bit_cast, an alternative approach may be to use arrays of PipelineDefBlank_t that are bit_casted at the end with return { .pl = std::bit_cast<typename FI_T::Pipelines>(...) ... };. (bit_casting an array to a struct of the same size)
src/osp/framework/framework.h
Outdated
| PipelineDefBlank_t plMembers[members_size] ; | ||
| std::memcpy(plMembers, &out.pl, sizeof(typename FI_T::Pipelines)); | ||
| //auto const plMembers = arrayCast<PipelineDefBlank_t>( arrayCast<std::byte>( arrayView(&out.pl, 1) ) ); | ||
| for (std::size_t i = 0; i < members_size; ++i) | ||
| { | ||
| plMembers[i].m_value = rInterface.pipelines[i]; |
There was a problem hiding this comment.
Why not memcpy directly with a pointer to plMembers[i].m_value instead of needing an intermediate array? wouldn't these assignments (including diMembers[i] = rInterface.data[i]; below) be the only lines that needs to be changed?
Assuming auto const plMembers was mistaken for an array? It's actually an ArrayView (pointer and size), not an array.
There was a problem hiding this comment.
I could test it.
I thought the cast to ArrayView was the problem. The compiler might understand a memcpy of size 4 exactly the same way it does a simple assignment.
What makes it work here for me is that the cast from opaque struct to array is done through memcpy and not ArrayView ?
src/osp/framework/framework.h
Outdated
| auto const plMembers = arrayCast<PipelineDefBlank_t>( arrayCast<std::byte>( arrayView(&out.pl, 1) ) ); | ||
| for (std::size_t i = 0; i < plMembers.size(); ++i) | ||
| constexpr size_t members_size = sizeof(typename FI_T::Pipelines)/sizeof(PipelineDefBlank_t); | ||
| PipelineDefBlank_t plMembers[members_size] ; |
There was a problem hiding this comment.
minor: I mentioned this intermediate array is likely not needed, though C-style arrays are generally avoided.
src/osp/framework/framework.h
Outdated
| if constexpr ( ! std::is_empty_v<typename FI_T::Pipelines> ) | ||
| { | ||
| auto const plMembers = arrayCast<PipelineDefBlank_t>( arrayCast<std::byte>( arrayView(&out.pl, 1) ) ); | ||
| for (std::size_t i = 0; i < plMembers.size(); ++i) | ||
| constexpr size_t members_size = sizeof(typename FI_T::Pipelines)/sizeof(PipelineDefBlank_t); |
There was a problem hiding this comment.
camelCase (though this variable may not be needed)
src/adera_app/features/jolt.cpp
Outdated
| @@ -623,7 +623,7 @@ static void rocket_thrust_force(BodyId const bodyId, ACtxJoltWorld const& rJolt, | |||
|
|
|||
| JPH::BodyID joltBodyId = BToJolt(bodyId); | |||
| Quaternion const rot = QuatJoltToMagnum(bodyInterface.GetRotation(joltBodyId)); | |||
| RVec3 joltCOM = bodyInterface.GetCenterOfMassPosition(joltBodyId) - bodyInterface.GetPosition(joltBodyId); | |||
| RVec3 joltCOM = bodyInterface.GetPosition(joltBodyId) - bodyInterface.GetCenterOfMassPosition(joltBodyId); | |||
There was a problem hiding this comment.
For the pendulum effect the rockets are having?
The issue here is that GetCenterOfMassPosition takes (world) rotation in account when it shouldn't. I worked this out on Capital-Asterisk@463fe6e , part of the commit I sent on the gdextension pull request. This can just be cherry-picked.
There was a problem hiding this comment.
Oh well perfect then.
src/osp/framework/framework.h
Outdated
| constexpr size_t members_size = sizeof(typename FI_T::Pipelines)/sizeof(PipelineDefBlank_t); | ||
| PipelineDefBlank_t plMembers[members_size] ; | ||
| std::memcpy(plMembers, &out.pl, sizeof(typename FI_T::Pipelines)); | ||
| //auto const plMembers = arrayCast<PipelineDefBlank_t>( arrayCast<std::byte>( arrayView(&out.pl, 1) ) ); |
There was a problem hiding this comment.
no need to keep commented out code
|
I changed the memcpy to just replace the array assignement. Is this what you suggested @Capital-Asterisk ? If that looks good I'll rewrite the commit history to remove the Jolt stuff and get a single commit. |
src/osp/framework/framework.h
Outdated
| } | ||
| } | ||
|
|
||
| if constexpr ( ! std::is_empty_v<typename FI_T::DataIds> ) | ||
| { | ||
| { |
There was a problem hiding this comment.
very minor: excess spacing
src/osp/framework/framework.h
Outdated
| if constexpr ( ! std::is_empty_v<typename FI_T::Pipelines> ) | ||
| { | ||
| auto const plMembers = arrayCast<PipelineDefBlank_t>( arrayCast<std::byte>( arrayView(&out.pl, 1) ) ); | ||
| for (std::size_t i = 0; i < plMembers.size(); ++i) | ||
| { | ||
| plMembers[i].m_value = rInterface.pipelines[i]; | ||
| memcpy(&plMembers[i].m_value, &rInterface.pipelines[i], sizeof(PipelineId)); |
There was a problem hiding this comment.
minor: std::memcpy. iirc, this is C's memcpy from <string.h>, instead of <cstring>
Lamakaio
force-pushed
the
aliasing_fix
branch
from
7eba42b to
5b555aa
Compare
|
Should be good ! |
A proposal for a fix of aliasing issues in framework, using some memcpy calls. They should be optimized away because the compiler is smart.
bit_cast also exists, but apparently it cannot be used to cast between arrays with elements of different size.
I bundled a small fix for jolt that was still pending, because I don't want to make a whole pull request for 1 line.