Closed JaXt0r closed 1 week ago
Hi Luis. Some findings:
GameConfiguration.cs
Bug: Barrier is rendered inside main menu (including thunder sound).
(Hint: If Bugs like these aren't related to you and occur in main already, just name it here and create a separate bug report, please.)
Tested on main: No barrier in main menu.
Bug: Oldmine (world switchings) isn't loading. I tested with main where it works properly. In this branch, the world meshes aren't rendered.
But the world chunks are there... Strange:
Bug: NPCs spawn one over another In main it's the case, whenever we set:
But in my case, I activated both:
I found the source: It's because inside NpcCreator.cs you still reference the FeatureFlag instead of the new GameConfiguration asset.
Hint: All your managers have an Init() function. How about naming an interface to ensure proper use? The interface method can also centrally describe why it's needed.
Or even better: Why not calling this function directly at construct()?
A I see... It seems we need to wait for Start() to initialize properly. Then back to my interface idea. ;-)
Ready for review. Please test: Everything :smile:
Most important bits:
GameConfiguration
) works?world.zen
work?
- You removed the SingletonBehaviour<?> from all/most of the *Manager.cs classes. Whenever we don't need more than 1 instance, we made classes static (Only reason was to show: Hey, I'm existing only once!). Is there a use case why this class isn't static? We could also make this class static and call an Init() function.
The idea here is to make the dependencies explicit. So, if a manager requires another manager to function correctly, we need to make sure that the other manager is properly initialized before its used. Passing them explicitly (-> dependency injection) helps make that dependency obvious to anyone inspecting the code.
- Some classes like GlobalEventDispatcher or IGlobalDataProvider don't sound like our classes. I suggest we think about a common prefix for class names, which sound like Unity ones. Goal: To distinguish easily between our logic and Unity's. I'm happy to change my mind, but want to discuss it with you first.
Hm maybe. I'd suggest we discuss a naming scheme internally and then add that to the style guide. Then we can rename all of the existing classes (since we'll probably need to rename others too).
- GameManager is handling all the Unity events like Update(), LateUpdate(), FixedUpdate(). My first idea was: The managers should take care of it on their own. Can you explore the benefits and drawbacks a little bit better, please. I'm on the fence right now.
With this setup, the managers cannot take care of this because they're not MonoBehaviour
s. This means, that this setup is required. It is also nice to be able to control the order in which the Update
(and so on) functions are called directly and make it visible immediately which managers need to be updated and which ones don't.
Hi @lmichaelis I thought about it and fully agree for the managers:
Please add a few information on the docs about the Manager handling and where they're initialized, then I'm fine about the three topics above.
I tested the main game including MainMenu, NPCs, and world switching. Except from the two texture issues (Textures of NPCs+Vobs in Lab scene + image frame in Gomez' throne room), everything works fine! Please go on.
Looks great. I will check with the lab issue. I also think it's related to my opened Unity.
Draft PR to track issues or change requests for new GUZ architecture.