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

Possible inconsistency between generated get_property and set_property! for Data #116

Closed
arunleob opened this issue Sep 27, 2024 · 2 comments · Fixed by #117
Closed

Possible inconsistency between generated get_property and set_property! for Data #116

arunleob opened this issue Sep 27, 2024 · 2 comments · Fixed by #117
Assignees
Labels
bug Something isn't working

Comments

@arunleob
Copy link

arunleob commented Sep 27, 2024

I ran into an issue trying to set time, i.e. doing the following

using MuJoCo
model, data = MuJoCo.sample_model_and_data()
data.time = 10.0
display(data.time) # Displays 0
display(unsafe_load(Ptr{Float64}(data.internal_pointer + 100))) # Displays 10.0

Looking at the generated wrapper, the getproperty function Base.getproperty(x::Data, f::Symbol) uses the following address to get time

f === :time && return unsafe_load(Ptr{Float64}(internal_pointer + 161616))

but the setproperty function Base.setproperty!(x::Data, f::Symbol, value) does

MuJoCo.jl/src/wrappers.jl

Lines 6264 to 6267 in 41df649

if f === :time
cvalue = convert(Float64, value)
unsafe_store!(Ptr{Float64}(internal_pointer + 100), cvalue)
return cvalue

Both seem to get the internal_pointer the same way

MuJoCo.jl/src/wrappers.jl

Lines 4824 to 4825 in 41df649

function Base.getproperty(x::Data, f::Symbol)
internal_pointer = getfield(x, :internal_pointer)

MuJoCo.jl/src/wrappers.jl

Lines 6171 to 6172 in 41df649

function Base.setproperty!(x::Data, f::Symbol, value)
internal_pointer = getfield(x, :internal_pointer)

Should these be the same? I tried manually calling unsafe_store!(Ptr{Float64}(data.internal_pointer + 161616), t) and that seemed to create the expected behavior for data.time = t. This may affect other fields in data and possibly other structs, I haven't checked. Thanks for your help!

I ran into this issue a while ago, but it hit again when trying to see why my visual global option to set the azimuth of the camera wasn't being loaded. Seems like the same thing may exists there as well

f === :azimuth && return unsafe_load(Ptr{Float64}(internal_pointer + 48))

if f === :azimuth
cvalue = convert(Float64, value)
unsafe_store!(Ptr{Float64}(internal_pointer + 20), cvalue)
return cvalue
end

@JamieMair JamieMair self-assigned this Oct 2, 2024
@JamieMair JamieMair added the bug Something isn't working label Oct 2, 2024
@JamieMair
Copy link
Owner

Hi @arunleob, thanks for the bug report, it was very helpful! This must have snuck in when updating to MuJoCo 3.

I've added a test for your example and it should be fixed now by #117. I will push the release after the CI is finished and the fix should be available later today.

@arunleob
Copy link
Author

arunleob commented Oct 9, 2024

Thank you, really appreciate the quick reply and fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants