search

rodrigoqueiroz / geoscenarioserver

9 stars 1 forks source link

Client unit conversion code mixed into the server side #116

Open icolwell-as opened 8 months ago

icolwell-as commented 8 months ago

I was just curious why CLIENT_METER_UNIT exists. Is there some reason we can't keep the shared memory API in base SI units, and each client can do its own conversions based on its own needs?

Similarly: https://github.com/rodrigoqueiroz/geoscenarioserver/blob/master/Actor.py#L165

I am thinking to refactor some of this stuff, but wanted to make sure I wasn't missing something.

rodrigoqueiroz commented 8 months ago

You are right. The Server should keep the original unit, and let the client do the conversion in both ways. I changed it locally, but never made it to the remote because it depends on other changes to the vehicle state for UE5 (I will push them when I have some free time).

Looking at my local changes, I also published the server yaw instead of yaw_unreal, and did the conversions in the new client. I also kept the original get_full_state_for_client() probably to not break WISE Sim. Maybe it is a good idea to keep it too if you refactor.

icolwell-as commented 8 months ago

Thanks @rodrigoqueiroz, here are the changes I did on our fork: https://github.com/icolwell-as/geoscenarioserver/pull/7/files Likely needs a bit of testing with unreal before trusting it (I'm not convinced the yaw adjustment in the unreal client is correct).

Happy to upstream the changes when it makes sense to. I'm only opening one upstream PR at a time, but let me know if you're interested in getting relevant PRs as soon as they're ready.