microsoft / AirSim

Open source simulator for autonomous vehicles built on Unreal Engine / Unity, from Microsoft AI & Research
https://microsoft.github.io/AirSim/
Other
16.07k stars 4.48k forks source link

Bug: Using python's simPlotPoints (not too) many times sometimes crashes the Unreal Engine #2959

Closed TomAvrech closed 3 years ago

TomAvrech commented 3 years ago

When running the below line, it works ok. self.client.simPlotPoints(points=[airsim.Vector3r(x_val=float(0), y_val=float(0), z_val=float(0))], color_rgba=[1.0, 0.0, 0.0, 1.0], size=1, duration=-1.0, is_persistent=True) But if I run it multiple times, -sometimes- an Exception is thrown: "UE4Editor-Win64-DebugGame.exe has triggered a breakpoint." in Unreal Projects\MyProject\Plugins\AirSim\Source\WorldSimApi.cpp line 321 After: 2639, 2846, 6117, and so on calls.... Notice: I run a constant, non-random code before this, always the same, but for some weird reason, it crashes, and also notice that the parameters I'm sending are very simple, and are almost the same as the basic usage example of simPlotPoints. Annotation 2020-08-22 095752

TomAvrech commented 3 years ago

I'm guessing this is some sort of a stack overflow. Is there a way to increase the memory size ?

rajat2004 commented 3 years ago

There was a somewhat similar problem earlier when using DrawDebugPoints in Lidar caused it to crash. Though seems strange that it's happening for such less no. of points. What happens if you set is_persistent to false, and use duration instead?

TomAvrech commented 3 years ago

There was a somewhat similar problem earlier when using DrawDebugPoints in Lidar caused it to crash. Though seems strange that it's happening for such less no. of points. What happens if you set is_persistent to false, and use duration instead?

I've just tried self.client.simPlotPoints(points=[airsim.Vector3r(x_val=float(0), y_val=float(0), z_val=float(0))], color_rgba=[1.0, 0.0, 0.0, 1.0], size=1, duration=10.0, is_persistent=False) Still the problem is there. I try to do it for 62500 points, which I don't think is a too large of a number.

TomAvrech commented 3 years ago

I tried 2 tricks that didn't work: 1) Added time.sleep() after each simPlotPoints call, to hopefully give the C++ enough time to allocate extra memory. 2) Make 1 call to simPlotPoints, before the real calls, which added <2 * real-number-of-points> "junk" points, so maybe the std::vector (if it is used at all) will not need to increase in size between real simPlotPoints calls.

rajat2004 commented 3 years ago

Could you test #2963, it seems to fix the problem for me, for 10k calls atleast My script -

import airsim
import time

client = airsim.VehicleClient()

points = 10000

start = time.time()

for i in range(points):
    client.simPlotPoints(points=[airsim.Vector3r(x_val=float(0), y_val=float(0), z_val=float(0))],
                        color_rgba=[1.0, 0.0, 0.0, 1.0], size=10, duration=-1, is_persistent=True)

    # if (i%(points//10)==0):
    #     client.simFlushPersistentMarkers()

# points_list = [airsim.Vector3r(x_val=float(0), y_val=float(0), z_val=float(0))] * points
# client.simPlotPoints(points=points_list, color_rgba=[1.0, 0.0, 0.0, 1.0], size=10, duration=-1, is_persistent=True)

end = time.time()
print(f"Time taken: {end-start}")

Side note from the timing result - for loop takes 21.05s, while the list one takes 0.027s

TomAvrech commented 3 years ago

Could you test #2963, it seems to fix the problem for me, for 10k calls atleast My script -

import airsim
import time

client = airsim.VehicleClient()

points = 10000

start = time.time()

for i in range(points):
    client.simPlotPoints(points=[airsim.Vector3r(x_val=float(0), y_val=float(0), z_val=float(0))],
                        color_rgba=[1.0, 0.0, 0.0, 1.0], size=10, duration=-1, is_persistent=True)

    # if (i%(points//10)==0):
    #     client.simFlushPersistentMarkers()

# points_list = [airsim.Vector3r(x_val=float(0), y_val=float(0), z_val=float(0))] * points
# client.simPlotPoints(points=points_list, color_rgba=[1.0, 0.0, 0.0, 1.0], size=10, duration=-1, is_persistent=True)

end = time.time()
print(f"Time taken: {end-start}")

Side note from the timing result - for loop takes 21.05s, while the list one takes 0.027s

What does GameThread mean ?

rajat2004 commented 3 years ago

Unreal Engine has different kinds of threads, I myself am not very clear about the exact differences, etc. From what I understand, game thread is the main one which runs on CPU, there's render thread, and Async operations also. This post pointed towards running on game thread so tried it out - https://answers.unrealengine.com/questions/286729/editor-crash-debugdrawpoint-in-fasynctask.html

Some info about threads (Need to read them myself :sweat_smile:)- https://docs.unrealengine.com/en-US/Programming/Rendering/ThreadedRendering/index.html, https://forums.unrealengine.com/development-discussion/c-gameplay-programming/85491-is-threading-what-i-think-it-is-and-what-s-up-with-async

TomAvrech commented 3 years ago

Unreal Engine has different kinds of threads, I myself am not very clear about the exact differences, etc. From what I understand, game thread is the main one which runs on CPU, there's render thread, and Async operations also. This post pointed towards running on game thread so tried it out - https://answers.unrealengine.com/questions/286729/editor-crash-debugdrawpoint-in-fasynctask.html

Some info about threads (Need to read them myself 😅)- https://docs.unrealengine.com/en-US/Programming/Rendering/ThreadedRendering/index.html, https://forums.unrealengine.com/development-discussion/c-gameplay-programming/85491-is-threading-what-i-think-it-is-and-what-s-up-with-async

I haven't modified anything in the C++ projected generated from the Epic Games Launcher. Should I ?

rajat2004 commented 3 years ago

Not exactly sure what you're saying, the change is fairly small so I guess you could even copy the new code and rebuild from VS

TomAvrech commented 3 years ago

Not exactly sure what you're saying, the change is fairly small so I guess you could even copy the new code and rebuild from VS

Annotation 2020-08-23 225814 Is that what you mean, adding mutex.lock.. ? (It didn't solve)

rajat2004 commented 3 years ago

No, try out the changes done in PR #2963

After the change, the code should be -

void WorldSimApi::simPlotPoints(const std::vector<Vector3r>& points, const std::vector<float>& color_rgba, float size, float duration, bool is_persistent)
{
    FLinearColor color {color_rgba[0], color_rgba[1], color_rgba[2], color_rgba[3]};
    UAirBlueprintLib::RunCommandOnGameThread([this, &points, &color, &size, &duration, &is_persistent]() {
        for (const auto& point : points) {
            DrawDebugPoint(simmode_->GetWorld(),
                    simmode_->getGlobalNedTransform().fromGlobalNed(point),
                    size, color.ToFColor(true), is_persistent, duration);
        }
    }, true);
}
TomAvrech commented 3 years ago

Overwritting WorldSimApi.cpp's WorldSimApi::simPlotPoints(..) with rajat2004's code solved it (for 62.5k points at least) !

Thanks a lot !

rajat2004 commented 3 years ago

Great, thanks a lot for reporting the bug and testing! I suspect this will require making such changes to all the plotting APIs. Might be a good chance to add some example scripts also

TomAvrech commented 3 years ago

Similar bug, similar fix: void WorldSimApi::simFlushPersistentMarkers() { UAirBlueprintLib::RunCommandOnGameThread([this]() { FlushPersistentDebugLines(simmode_->GetWorld()); }, true); }

rajat2004 commented 3 years ago

Yeah, I made the changes to all the APIs but forgot this one, added now, thanks!