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

GD.Seed C# Black Screen HTML5 #36858

Closed
Ivan-Tigan opened this issue Mar 6, 2020 · 10 comments · Fixed by #44105
Closed

GD.Seed C# Black Screen HTML5 #36858

Ivan-Tigan opened this issue Mar 6, 2020 · 10 comments · Fixed by #44105

Comments

@Ivan-Tigan
Copy link

Ivan-Tigan commented Mar 6, 2020

Godot version:
Mono 3.2, 3.2.1 rc, 3.2.2 rc (I tested it with all of them as they were getting released to make sure it's not fixed)

OS/device including version:
Linux (pop-os), HTML5 in Google Chrome

Issue description:
C#/F# GD.Seed(1UL); (same for GD.Seed(1);)
produce black screen of death in HTML build.
The black screen does not get produced when doing a Linux build.
Only when it is in the browser.
Moreover, the issues does not occur if you call seed from GDScript.

Steps to reproduce:

  1. Create scene
  2. Add C# script to the scene
  3. Inside ready call GD.Seed(1); or GD.Seed(1UL);

Test in Browser => Black Screen
Test Normally => Works fine
5. (Optional) Try same thing with GDScript => works fine both in browser and normally
Project zip at the bottom after pics.

Minimal reproduction project:
image
image
seed_bug.zip

@Ivan-Tigan
Copy link
Author

Ivan-Tigan commented Mar 6, 2020

Also the engine really needs a way to see console output when testing HTML5 projects especially when you get a black screen -- debugging seems impossible in those cases as the black screen is only in HTML not in the normal mode.
Should I make a feature request?

@Calinou
Copy link
Member

Calinou commented Mar 6, 2020

Can you reproduce this when doing the same operation, but in GDScript with non-Mono export templates?

Also the engine really needs a way to see console output when testing test HTML5 especially when you get a black screen - debugging seems impossible.

Can't you see the output in the web inspector by pressing F12 or Ctrl + Shift + I?

@Ivan-Tigan
Copy link
Author

Ivan-Tigan commented Mar 6, 2020

Also the engine really needs a way to see console output when testing test HTML5 especially when you get a black screen - debugging seems impossible.

Can't you see the output in the web inspector by pressing F12 or Ctrl + Shift + I?

Yes, I think in the console tab. I didn't know that. Thanks!
I will leave the question if anyone else stumbles upon it.
(In this case the output isn't the most helpful haha)

Ok, after saying that. I could only open the Inspector with the black screen (crashed) game. I can't seem to be able to open it when the game actually works. I can however right click before the game has loaded so that allows me to open it but I have to be very quick.

@neikeq
Copy link
Contributor

neikeq commented Mar 6, 2020

Did you try with ( ⋮ ) > More tools > Developer tools?

@Ivan-Tigan
Copy link
Author

Did you try with ( ⋮ ) > More tools > Developer tools?

Absolutely perfect! Thanks a lot!

@neikeq
Copy link
Contributor

neikeq commented Mar 6, 2020

I think I know why this is happening. Mono on WASM needs to generate a trampoline for each internal call function signature possible for the interpreter: https://github.com/mono/mono/blob/master/mono/mini/wasm_m2n_invoke.g.h

The trampolines they generate are not enough for all of Godot's internal call signatures. This was addressed in this commit with a temporary workaround: 8cbe4a3

Also now we pass and return long, ulong, float and double as ref parameters as well. This is due to missing trampolines for float and long types. This is likely a temporary workaround that will be reverted in the future. The correct solution would be to patch 'mono/mini/m2n-gen.cs' when building the Mono runtime for WASM in order to generate the trampolines we need.

However those changes were only applied to the generated bindings. We need to do the same for the ones we maintain manually, like GD.Seed in this case.

Although at this point, it may be better to revert the workaround and go with the proper solution mentioned above.

@neikeq neikeq self-assigned this Mar 6, 2020
@neikeq neikeq added this to the 4.0 milestone Mar 6, 2020
@Faless
Copy link
Collaborator

Faless commented Dec 11, 2020

@neikeq Is this fixed via: godotengine/godot-mono-builds#21 ? Or does we still need proper binding for GD.Seed?

@neikeq
Copy link
Contributor

neikeq commented Dec 13, 2020

If the cause of the issue is what I suspected in my previous comment, then #44105 should fix it. However, that PR cannot be backported to 3.2 as it requires C++17. @akien-mga would it be possible to make an exception about the C++ version for the Mono module in 3.2? Otherwise the solution is to fix GD.Seed and any other internal calls that could have the same issue.

@akien-mga
Copy link
Member

@akien-mga would it be possible to make an exception about the C++ version for the Mono module in 3.2?

The 3.2 branch builds with C++14 currently. We could enable C++17 but that would increase the minimum requirements on supported compilers (not a problem for the official buildsystem but might be from some users). I'm not sure what other impact it would have aside from enabling the variadic templates you need.

Otherwise the solution is to fix GD.Seed and any other internal calls that could have the same issue.

If it's a simple change, I'd say that it would be worth doing first regardless of whether #44105 can be backported, as that one is not merged yet and will likely require some testing to validate the changes. For the 3.2 branch if there's an immediate safe solution, that's always good to take.

@neikeq
Copy link
Contributor

neikeq commented Dec 14, 2020

Confirmed it's the issue with the missing trampolines and #44105 fixes it (#44373 for the 3.2 branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants