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

Fix Weather API crash #3009

Merged
merged 2 commits into from
Jul 23, 2021
Merged

Fix Weather API crash #3009

merged 2 commits into from
Jul 23, 2021

Conversation

rajat2004
Copy link
Contributor

@rajat2004 rajat2004 commented Sep 7, 2020

Fixes: #2988

With the latest release binary, using the simEnableWeather API causes crash, with the following error -

Assertion failed: IsInGameThread() [File:/home/ue4/UnrealEngine-4.24.2-linux-all-glsl/Engine/Source/Runtime/CoreUObject/Private/UObject/UObjectGlobals.cpp] [Line: 1065] 
Unable to load /AirSim/Weather/WeatherFX/WeatherGlobalParams. Objects and Packages can only be loaded from the game thread.

Note that it'll work fine in the Editor, with or without the fix and the next problem described below

Quite clear, and so added the RunOnGameThread part, which exposes the more serious problem, the Weather assets are not being packaged
Issue which brought this problem to light - #2988 (Described in more detail as well)

There is a workaround mentioned in the issue, but I think it'll be better to fix this at the root itself (unless turns out to be too difficult)
Haven't yet figured out how to load the asset correctly using C++ though

A Linux binary with the game thread fix applied - https://drive.google.com/file/d/1kEEhSq0u5UGiCQs3bnlZKENx7A1cnW4Q/view?usp=sharing
Current error message -

[2020.09.07-11.34.37:392][ 50]LogStreaming: Error: Couldn't find file for package /AirSim/Weather/WeatherFX/WeatherGlobalParams requested by async loading code. NameToLoad: /AirSim/Weather/WeatherFX/WeatherGlobalParams
[2020.09.07-11.34.37:392][ 50]LogStreaming: Error: Found 0 dependent packages...
[2020.09.07-11.34.37:392][ 50]LogUObjectGlobals: Warning: Failed to find object 'MaterialParameterCollection None./AirSim/Weather/WeatherFX/WeatherGlobalParams'
[2020.09.07-11.34.37:392][ 50]LogTemp: Warning: Warning, WeatherAPI could NOT get WeatherParameterCollection1!
[2020.09.07-11.34.37:392][ 50]LogTemp: Warning: Warning, WeatherAPI could NOT get MaterialCollectionInstance!

@jonyMarino jonyMarino added the bug label Oct 5, 2020
@zimmy87
Copy link
Contributor

zimmy87 commented Dec 18, 2020

Hey @rajat2004 what other tasks are remaining for this PR? Do you have an estimate for when this will come out of the draft stage?

@rajat2004
Copy link
Contributor Author

@zimmy87 The main problem right now to work on loading the assets correctly, currently they get removed from the packaged build since UE can't find any references to them, with the error message posted above

[2020.09.07-11.34.37:392][ 50]LogStreaming: Error: Couldn't find file for package /AirSim/Weather/WeatherFX/WeatherGlobalParams requested by async loading code. NameToLoad: /AirSim/Weather/WeatherFX/WeatherGlobalParams
[2020.09.07-11.34.37:392][ 50]LogStreaming: Error: Found 0 dependent packages...
[2020.09.07-11.34.37:392][ 50]LogUObjectGlobals: Warning: Failed to find object 'MaterialParameterCollection None./AirSim/Weather/WeatherFX/WeatherGlobalParams'
[2020.09.07-11.34.37:392][ 50]LogTemp: Warning: Warning, WeatherAPI could NOT get WeatherParameterCollection1!
[2020.09.07-11.34.37:392][ 50]LogTemp: Warning: Warning, WeatherAPI could NOT get MaterialCollectionInstance!

I'll mostly be able to work on this only after a few days, have some exams right now

@rajat2004
Copy link
Contributor Author

@saihv If possible, could you have a look at this PR when free? I'm having some troubles with the Weather assets, the implementation seems to be in both C++ code and .uasset files, I refreshed the assets which UE said had compilation errors, I feel things should be working if it were using just the C++ code, however only some weather effects such as Fog or Dust are working, and others are not. I'll also try to dig more into what is causing the issues, but this might become too complex if changes are required in both code and assets.
Thanks!

Copy link
Contributor

@zimmy87 zimmy87 left a comment

Choose a reason for hiding this comment

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

No rush on these changes as this is still in the draft stage; just leaving these comments for when you're ready to move out of draft stage

Unreal/Plugins/AirSim/Source/SimMode/SimModeBase.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/SimMode/SimModeBase.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Weather/WeatherLib.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Weather/WeatherLib.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Weather/WeatherLib.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Weather/WeatherLib.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/Weather/WeatherLib.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/WorldSimApi.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/WorldSimApi.cpp Outdated Show resolved Hide resolved
@zimmy87
Copy link
Contributor

zimmy87 commented Mar 15, 2021

Hi @rajat2004, as per loading the assets, is there 1 or more objects that can be added to "AirSim\Unreal\Environments\Blocks\Content\AirSimAssets.umap" that will force the right assets to load for that project? In all the release environments, we add a copy of AirSimAssets.umap to the environment's content folder, which forces the assets for both the drone and car to load (otherwise we get similar errors about the assets not being packaged).

@rajat2004
Copy link
Contributor Author

@zimmy87 Yeah, I believe adding the Weather assets to the map should force UE to package them as well. If that works, then just the first commit should be enough to fix the problem.
BTW, is AirSimAssets.umap the file you're referring to? That one hasn't been updated in 3 years, and you had mentioned in some other threads also that the level map has been updated to include all assets to fix the missing assets errors in the releases. It seems to be an environment-specific file, just wondering, is it possible to have such a file for just the AirSim plugin itself? I think it'll allow packaging the binaries if required by the user such as from the master or using a custom environment.

TBH, I personally would prefer that this problem be fixed in the code itself, but that seems to be getting pretty messy, so including the assets is probably the easier way to go

@zimmy87
Copy link
Contributor

zimmy87 commented May 26, 2021

Hi @rajat2004, we've published #3729 which relocates AirSimAssets.umap to be inside of the plugin. If there's a actor that can be added to AirSimAssets.umap that will fix the packaging errors you're seeing, let us know and we can update our PR.

@jonyMarino
Copy link
Collaborator

Hi @rajat2004! Is there something you need to continue with this PR?

@rajat2004
Copy link
Contributor Author

@jonyMarino I think the only fix required now is to run the weather commands on the game thread, the missing assets can be fixed by including them in the map which references all the assets. I'll drop the later commits, if they're needed it'll better to do it again from scratch especially with the assets involved.

@rajat2004 rajat2004 marked this pull request as ready for review July 14, 2021 19:10
@jonyMarino
Copy link
Collaborator

Hi Rajat! It's weird, but I've tested your script (weather.py) without your changes, and it seems to work fine with the latest version (1.5), even without the change David made to AirSimAssets.umap.
I don't know what would have solved it.
Is there another reason why we should merge this?

@rajat2004
Copy link
Contributor Author

@jonyMarino Did you try running the script with a binary rather than the editor? It fails with the 1.5.0 release for me. Currently can't compile and test a binary

@zimmy87
Copy link
Contributor

zimmy87 commented Jul 23, 2021

This turned out to be a Linux-only issue. I was able to repro with both binaries and in editor, but only in Linux. Applying this PR fixed the issue so I am moving ahead with merging. Thanks for the contribution @rajat2004!

@zimmy87 zimmy87 merged commit a072a1b into microsoft:master Jul 23, 2021
@rajat2004 rajat2004 deleted the weather-api-crash branch July 24, 2021 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using simEnableWeather(True) causes crash
4 participants