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

[ue4] Hot Reloading with a character in the scene results in a crash #1246

Closed
Drew-Clowery opened this issue Jan 10, 2019 · 22 comments
Closed
Assignees

Comments

@Drew-Clowery
Copy link

Drew-Clowery commented Jan 10, 2019

A crash occurs with 100% frequency if you reimport the asset data of a spine skeleton while a pawn/actor with that skeleton is in the currently loaded level.

Repro Steps:

  1. Open the SpineUE4 project
  2. Open the Basic Animation level (Content/GettingStarted/01-basic-animation)
  3. Open the spineboy Skeleton Data asset details (double click on the asset)
  4. Reload the asset data (Asset->Reimport Spineboy-data)
  5. Hit Play
  6. Hit escape or stop.

Results:
The UE4 editor crashes due to uncaught exceptions

Additional Notes:
It appears this is related to Issue #1180 (Certainly the issue wasn't exposed until we could hot load skeleton data). The uncaught exceptions occur at SpineSkeletonRendererComponent.cpp line 220. It appears that the some or all of the Slots/Attachments in the skeleton have been partially cleaned up.

@badlogic badlogic changed the title Hot Reloading with a character in the scene results in a crash [ue4] Hot Reloading with a character in the scene results in a crash Jan 10, 2019
@badlogic badlogic self-assigned this Jan 10, 2019
@badlogic badlogic added the bug label Jan 10, 2019
@badlogic
Copy link
Collaborator

Thanks for reporting. It is indeed related to #1177 for which I haven't found a solution yet. I'm closing #1177 in favor of this more descriptive repro.

@badlogic
Copy link
Collaborator

@Drew-Clowery I've tried to reproduce the issue as per your reproduction steps using UE 4.21.1 and the latest spine-ue4/spine-cpp from the 3.7 branch from today on Windows 10 with Visual Studio 2017.

I've tried both re-importing the assets as outlined by you, and also by overwriting the asset source file (spineboy.json). Both things worked as expected without the editor crashing.

Could you maybe give me more information? What Spine Runtimes branch are you working off of? What Visual Studio version are you using? What VS build configuration are you using? I'm using "Development Editor" on my end.

@Drew-Clowery
Copy link
Author

Drew-Clowery commented Jan 18, 2019

Sorry for the slow reply. I have just reproduced this using the 3.7 Spine runtimes, Unreal Engine 4.21.1, Visual Studio 17 Community v15.9.4 on Windows 10.

My exact repro steps were:

  1. Download the spine-runtimes-3.7.zip from http://esotericsoftware.com/git/spine-runtimes/archive
  2. Extract the files from that .zip to my desktop.
  3. Copy spine-runtimes-3.7/spine-cpp/spine-cpp to spine-runtimes-3.7/spine-ue4/Plugins/SpinePlugin/Source/SpinePlugin/Public
  4. In spine-runtimes-3.7/spine-ue4 right click on spine-runtimes-3.7/spine-ue4/SpineUE4.uproject and select "Generate Visual Studio Project Files"
  5. Open the generated spineUE4.sln file with Visual Studio
  6. In Visual Studio select Debug->Start Debugging
  7. Wait for UE4 editor to open with the SpineUE4 project
  8. Exited fullscreen mode in the UE4 editor (I doubt this matters, but fullscreen is the devil)
  9. Selected File->Open Level in the UE4 Editor
  10. In the Open Level Dialog, double clicked on "GettingStarted" then selected the "01-basic-animation" level.
  11. In the Open Level Dialog clicked Open to open the selected level.
  12. Selected the SpineboyCppPawn1 actor in the World Outliner
  13. Selected SpineSkeletonAnimation component in the details window
  14. Clicked on the magnifying glass icon next to Skeleton Data in the Spine section of the Details window in order to show the location of that file in the content browser
  15. Double clicked on spineboy-data in the content browser to open the asset details window
  16. Selected Assets->Reimport spineboy-data... in the asset details window
  17. Selected "Yes All" on the two pop-up messages that appear.
  18. Closed the Asset details window.
  19. Clicked "Play" in the main UE4 Editor Window
  20. Clicked "Stop" in the main UE4 Editor Window

At that point Visual studio hit an uncaught exception at USpineSkeletonRendererComponent.cpp line 220.

I will attempt to record a video of these steps in a moment and attach it to this bug.

@badlogic
Copy link
Collaborator

This are my exact reproduction steps. Looks like we have an indeterministic memory corruption on our hands. I'll try to get this to crash on my end.

@Drew-Clowery
Copy link
Author

Drew-Clowery commented Jan 18, 2019

So my assessment was that this was happening because the Actor in the level was not properly cleaned up, or cleaned up too early. I haven't stepped through it in exacting detail, but a little quick debugging showed that the attachment and maybe the slot were both partially or completely cleaned-up.

This repros at 100% with me, I've run it over a dozen times now, so I don't think it's indeterministic.

@Drew-Clowery
Copy link
Author

I've uploaded a video of my repro: https://youtu.be/IL9nSbCEcvw

@badlogic
Copy link
Collaborator

badlogic commented Jan 18, 2019

Thanks, this is essentially what I've been trying. The indeterminism stems from the fact that memory management even in debug builds is not deterministic. On some machines, access to freed memory crashes immediately, either due to a trap or because the data has been overwritten and is thus invalid (as in your case, where a pointer dereference fails). On some machines (like mine), the same build may still have the bug, but the segfault trap is not triggered, nor is the freed memory reused and overwritten.

Sometimes things change because of a reboot, or because other programs are running. In any case, there's clearly a bug, and it definitely looks like a use-after-free bug. I think VS has some memory debugging tools I can try to see where this use-after-free error bug from.

Thank you a lot for helping out with this!

@Drew-Clowery
Copy link
Author

Ahh, I understand what you are saying now.

Thank you very much for working on this, and please let me know if there's anything else I can do to help.

@scardario
Copy link

scardario commented Jan 20, 2019

Hi, I just ran into this issue, this is what Unreal editor shows at the moment it crashes:

`Access violation - code c0000005 (first/second chance not available)

UE4Editor_SpinePlugin!spine::PathConstraint::update() [d:\proyectos\nachosadventures\support\unreal\spineruntimecompile\plugins\spineplugin\source\spineplugin\public\spine-cpp\src\spine\pathconstraint.cpp:78]
UE4Editor_SpinePlugin!spine::Skeleton::updateWorldTransform() [d:\proyectos\nachosadventures\support\unreal\spineruntimecompile\plugins\spineplugin\source\spineplugin\public\spine-cpp\src\spine\skeleton.cpp:215]
UE4Editor_SpinePlugin!USpineSkeletonAnimationComponent::InternalTick() [d:\proyectos\nachosadventures\support\unreal\spineruntimecompile\plugins\spineplugin\source\spineplugin\private\spineskeletonanimationcomponent.cpp:102]
UE4Editor_Engine
UE4Editor_Engine
UE4Editor_Engine
UE4Editor_Core
UE4Editor_Core
UE4Editor_Engine
UE4Editor_Engine
UE4Editor_Engine
UE4Editor_Engine
UE4Editor_UnrealEd
UE4Editor_UnrealEd
UE4Editor
UE4Editor
UE4Editor
UE4Editor
UE4Editor
kernel32
ntdll`

I restarted the editor, deleted the only actor using the asset and it reimported without problem.

Using 4.20.3 with runtime 3.7, updated 2 days ago.

@badlogic
Copy link
Collaborator

@scardario to be clear, this happened when you tried to re-import assets as described above?

@scardario
Copy link

@scardario to be clear, this happened when you tried to re-import assets as described above?

Mmmhh, sorry, not exactly, after reading more carefully I realize my issue is a bit different:

  1. In the Unreal Editor Content Browser I open the folder I have the Spine assets already imported. Then I click on "import" button to bring the updated assets.
  2. I choose "yes to all" on every option.
  3. The editor crashes while importing, I don't get to do anything more that that.

It seems to be related to having an actor using the asset while importing, since it worked when I deleted from the level the only actor using it.

I apologize if this is not the correct place to post this problem.

@badlogic
Copy link
Collaborator

@scardario no, this is the perfect place for the report! Thanks!

Some good news: I think I have identified the issue (documenting it here for when I return to the code tomorrow):

Both SpineAtlasAsset and SpineSkeletonDataAsset are affected the same way. Each has an import factory responsible for reading the raw data from .atlas and .json files respectively. In both cases, that data gets stored in a field (FString and TArray<uint8>). Both classes can construct spine::Atlas and spine::SkeletonData instances when asked nicely via GetAtlas() and GetSkeletonData(). The returned instances will reference memory in the raw data.

All this works fine when a previously imported asset is loaded for a scene to construct SpineSkeletonAnimationComponent and SpineSkeletonRendererComponent components. The raw data is essentially static. Until we re-import the data. That's when things go wrong.

The first problem is, that the factories for the two asset types simply override the existing raw data, prompting a deallocation of the old raw data. Any spine::Atlas and spine::SkeletonData instances created from the old data now points at invalid memory.

Worse, upon re-import, the SpineSkeletonAnimationComponent doesn't re-construct the spine::XXX instances based on the new raw data in the assets. It simply has no way to know that the underlying data changed.

It will take me a bit to rewrite how all of this works. In the end, we should have a working, cleaner version of the current setup.

Again, thanks to @Drew-Clowery and @scardario for reporting this and helping out with steps and stack traces. The clue to all these non-obvious side effects of reimporting assets actually came from your stack traces. Without those, I would not have been able to identify where things go wrong. Cheers!

@scardario
Copy link

Glad to be of any help.
Let me know if you need something else.

@badlogic
Copy link
Collaborator

@Drew-Clowery @scardario the latest commit should fix the re-import issue you are seeing. While I was not able to reproduce the error locally, this new way of handling (re-)imports should be stable.

Please try out the changes in the latest 3.7 or 3.8-beta branch, and let me know if you run into issues with it.

Thank you both for helping out with this!

@scardario
Copy link

Hi, I couldn't compile the new version on 4.20.3 using branch 3.7: I get this error in a lot of files:

Plugins\SpinePlugin\Source\SpinePlugin\Private\SpineAtlasAsset.cpp(1): error : Expected SpineAtlasAsset.h to be first header included.

I know this should be easy to figure out, but I don't work with C++. I already tried twice to check if I was making some mistake following the runtime compile instructions.

image

@badlogic
Copy link
Collaborator

@scardario did you also update the build.cs files of the plugin?

@scardario
Copy link

scardario commented Jan 23, 2019

I updated all the github project, deleted the spineplugin folder from plugins and copied it again. (I have the repository in a separate folder)

If you're asking about the project build.cs. No, I usually don't modify it in order to compile spinePlugin.
I tried to though and added SpinePlugin to PublicDependencyModuleNames, but didn't find PublicIncludePaths to add SpinePlugin/Public and SpinePlugin/Classes (step 2 of these instructions) and the sample build.cs doesn't show where.

The result so far with SpinePlugin added to PublicDependencyModuleNames is the same list of errors.

I'll try to get all the files again, maybe my git client didn't update all the files.

@badlogic
Copy link
Collaborator

badlogic commented Jan 31, 2019 via email

@scardario
Copy link

No problem, I've had a busy week too.

I'm using unreal 4.20.3, trying to compile main branch - 3.7

I deleted the repository from my disk and cloned it again, and copied the sample project, followed instructions (copy spine-cpp folder to ... etc) and the results are the same.

The same errors, at least the same type:

spine-ue4\Plugins\SpinePlugin\Source\SpinePlugin\Public\spine-cpp\src\spine\Bone.cpp(1): error : Expected Bone.h to be first header included.

@badlogic badlogic reopened this Feb 1, 2019
@badlogic
Copy link
Collaborator

badlogic commented Feb 1, 2019

Oki, I'll give those steps a try on Monday. FWIW, I'm on 4.21.2 here.

@badlogic badlogic added the ready label Feb 1, 2019
@badlogic
Copy link
Collaborator

I've tried to reproduce this without success. @scardario could you figure out a solution to this on your end?

@badlogic
Copy link
Collaborator

badlogic commented Mar 6, 2019

I see @scardario has posted new UE4 forum threads, I assume you have figured out the include path issue. Closing this.

@badlogic badlogic closed this as completed Mar 6, 2019
@badlogic badlogic removed the ready label Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants