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

Remove workaround for empty struct in RPC #2812

Merged
merged 1 commit into from
Dec 3, 2020

Conversation

rajat2004
Copy link
Contributor

@rajat2004 rajat2004 commented Jun 30, 2020

rpclib/rpclib#152 has been fixed
Testing was with #2810 where empty Lidar data was produced sometimes, no crash, and can be checked easily in Python e.g.
if not lidar_data.point_cloud:

@madratman
Copy link
Contributor

/azp run microsoft.AirSim

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@madratman
Copy link
Contributor

Thanks for this. Does this PR affect the client side behavior when empty image is returned.
(yes, the client side would need to check for the same, as is the case now. But my question is if you've tested the same for images as well?)

@rajat2004
Copy link
Contributor Author

The client-side behaviour which would change is that it'll be possible to check if the image is empty with if not image_data_uint8 or something, or the always present way to check if length matches with HxWxC. This will affect a check of whether the length is 1 or not, but that's wrong to begin with. Will test with images to confirm.
This does give an idea to add a utility function to check if image is empty or not

@rajat2004
Copy link
Contributor Author

Well, it's surprisingly difficult to get empty images, fetched hundreds of images, even cranked up the res to 4k but still no empty images. Great that it happens so rarely but makes it that much harder to reproduce.
If anyone is able to test or has any ideas, do tell me. A possible way could be to add code to randomly clear the vector when constructing the RPC message

Copy link

@tool3210 tool3210 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zimmy87
Copy link
Contributor

zimmy87 commented Dec 3, 2020

Tested this locally with the Lidar scenario and it worked for me. I'm also unsure of how to test this with images. I tried configuring a camera to have 0 width and 0 height, but that generates an exception in the camera setup logic.

@rajat2004
Copy link
Contributor Author

@zimmy87 Thanks for testing! I haven't tested more than my previous comment indicated, but feel that the idea mentioned there to add a piece of code which randomly does not put the image in should work. Will try to test it out soon

@zimmy87
Copy link
Contributor

zimmy87 commented Dec 3, 2020

I hard-coded the ImageResponse to have empty image data and the simGetImages API still worked for me, so I think this is a safe enough fix to merge.

@zimmy87 zimmy87 merged commit f6553d4 into microsoft:master Dec 3, 2020
@rajat2004
Copy link
Contributor Author

Great, thanks a lot for testing!
If you don't mind, could you have a look at #3180 and see whether GitHub Actions needs to be enabled or something? It would give at least some basic CI checks until Azure Pipelines is up again which can do the entire build with UE4

@rajat2004 rajat2004 deleted the rpc-empty-struct branch December 3, 2020 22:31
@jonyMarino
Copy link
Collaborator

Hi Rajat! Will do. I think the Azure pipelines are working. We didn't call it here because Zimmy compiled in his local machine.

@rajat2004 rajat2004 mentioned this pull request Dec 6, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants