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

Refactor Global State #39

Merged
merged 50 commits into from
Jun 28, 2024
Merged

Conversation

JaXt0r
Copy link
Contributor

@JaXt0r JaXt0r commented Jun 17, 2024

Draft PR to track issues or change requests for new GUZ architecture.

…he` into `ResourceLoader`

`ResourceLoader` is a far more obvious name for a class which loads assets from disk. It also makes a lot of sense to move basically everything that has to do with loading these assets into one class to avoid scattering it across many files.
… callees

Additionally, touches `WaynetCreator`, `VobCreator` and `WorldCreator`
@JaXt0r
Copy link
Contributor Author

JaXt0r commented Jun 17, 2024

Hi Luis. Some findings:

GameConfiguration.cs

  • I think it should reflect the production values we use for build. This makes it super easy to recreate the asset if there's a new field.
  • Please let us store the prod-one into the Resource folder by default. I think of: Assets/Gothic-UnZENity-Core/Resources/GameConfigurations/GUZ-Features-Prod
  • Changes to the GameConfiguration.cs to reflect prod values:
    • showMeshCullingGizmos = false
    • StartHour + StartMinute=08:00 <-- I'd prefer the official G1 start time. Makes it easier to debug "New Game" issues in the future.
    • sunLightColor = ... <-- I think it was a different color in FeatureFlags. Some yellow-ish one?
    • spawn specific Npcs <-- Field is missing in your Scriptable object.
    • enableDecalVisuals/enableParticleEffects = false <-- These are experimental / WIP features. Maybe we highlight those in a separate group?
  • I like the new grouping based on Developer/Audio/Lighting/...

This is a weird issue I only found through `git bisect`. It must have something to do with the shaders but really? I have to call `InitSky` AFTER I'm done settings up the meshes? How peculiar.
@lmichaelis lmichaelis self-assigned this Jun 22, 2024
@lmichaelis lmichaelis changed the title @lmichaelis/refactor/dependency injection Refactor Global State Jun 22, 2024
@lmichaelis lmichaelis marked this pull request as ready for review June 22, 2024 09:31
@lmichaelis
Copy link
Contributor

lmichaelis commented Jun 22, 2024

Ready for review. Please test: Everything 😄

Most important bits:

  • Game starts?
  • Lab starts and works?
  • Game nenu works?
  • Lighting correct?
  • Sky okay?
  • Barrier works?
  • XR interactions okay?
  • Configuration (via GameConfiguration) works?
  • Worlds other than world.zen work?
  • Transitions between worlds work?
  • Works on XR hardware?
  • Release works and build runs?

JaXt0r

This comment was marked as off-topic.

Copy link
Contributor Author

@JaXt0r JaXt0r left a comment

Choose a reason for hiding this comment

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

Hi @lmichaelis . Nicely done. I added my remarks at the specific code parts. Feel free to comment, use, or decline at your own judgement.

Tested:

  • Lab

Open to test:

  • Normal game

A few general questions about the source code:

  1. 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.
  2. 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.
  3. 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.

@lmichaelis
Copy link
Contributor

lmichaelis commented Jun 24, 2024

  1. 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.

  1. 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).

  1. 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 MonoBehaviours. 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.

@JaXt0r
Copy link
Contributor Author

JaXt0r commented Jun 25, 2024

Hi @lmichaelis I thought about it and fully agree for the managers:

  1. Let's stick with your DI idea. I see your point in using the proper init order and centralized initialization of these singleton managers. It can also have benefits in the future if we want to overwrite manager classes for specific behaviour (e.g. G1/G2, XRIT/HVR, ...).
  2. It might also be just me who's unconfident with this. In the end, I will get used to it once settled. The more I think, the more I dislike prefixes as they're redundant and make class/file names even longer. Feel free to suggest some naming schema or we just go on with your options for now (Especially as the new classes are in the root folder of Core/Scripts and easily seeable.)
  3. Ok. Understood. Let's stick with this central management of Managers (what a phrase 🤣). We need to act decentralized for Components (e.g. colliders on NPCs) but this is another topic and has nothing to do with the managers' centralized handling itself.

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.

@JaXt0r
Copy link
Contributor Author

JaXt0r commented Jun 25, 2024

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.

…njection

# Conflicts:
#	Assets/Gothic-UnZENity-Core/Scripts/Caches/PrefabCache.cs
#	Assets/Gothic-UnZENity-Core/Scripts/Manager/XRDeviceSimulatorManager.cs
#	ProjectSettings/ProjectSettings.asset
@JaXt0r
Copy link
Contributor Author

JaXt0r commented Jun 28, 2024

Looks great. I will check with the lab issue. I also think it's related to my opened Unity.

@JaXt0r JaXt0r merged commit b303b91 into main Jun 28, 2024
@JaXt0r JaXt0r deleted the @lmichaelis/refactor/dependency-injection branch June 28, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants