Dynamically add player HUD to map scenes

aronand commented 2 months ago

As far as I can tell, currently the HUD has to be added to each map scene for it to work/exist. I propose that it is instead instantiated during scene loading (either in CogitoSceneManager or in CogitoScene), or/and made into a singleton as we'll most likely not have or need multiple instances of the HUD for the player.

Going with a singleton would open new possibilities for updating data as well. This could possibly allow us to signal (some) user interface changes directly via the singleton.

Singleton pseudocode

## UserInterface singleton
var ui_scene = preload("res://.../Player_HUD.tscn").instantiate()

## Scene or SceneManager
scene.add_child(UserInterface.ui_scene)

aronand commented 2 months ago

Added a quick implementation as a draft PR.

Phazorknight commented 2 months ago

Haven't looked at the PR yet, but just wanted to mention that it has previously come up to streamline this a bit more in general. Some ideas where to add the Pause Menu and Player HUD to the Player scene itself, another approach was to spawn or instance all three of these dynamically at runtime and using the CogitoScene settings for stuff like spawn points. Will heavily depend on complexity vs versatility.

Ok, will look into your PR now.

aronand commented 2 months ago

Yup, the PR is a quick and dirty change to see if it works, and does just what you described with the HUD and pause menus. The singleton preloads both, and then CogitoScene instantiates them and adds them into the scene.

It's a very small change and visually works, however one thing to verify is that the scene management doesn't work any magic behind the scenes, i.e. ensure there ever is only one instance of both UI elements in the scene, as in one of the variations I tested only the initial scene had the HUD, and this didn't pass into the other scenes.

For my own project I ended up doing the following:

class_name UserInterfaceSingleton

static var _instantiated: bool = false
var _ui_scene: PackedScene = preload("res://core/user_interface.tscn")
var _ui_instance: Control = null
var ui_instance: Control:
    get:
        if _ui_instance == null:
            _ui_instance = _ui_scene.instantiate()
        return _ui_instance

func _init() -> void:
    assert(not _instantiated, "Do not instantiate UserInterfaceSingleton!")
    _instantiated = true

The null check is there, because a scene change seems to also free the instance, even if it comes from the singleton. I didn't too heavily investigate this however, it might be possible to simply reparent the UI scenes instead 🤔

And then the scene simply gets the instance from the singleton:

func _ready() -> void:
    var ui: Control = Icarus.UserInterface.ui_instance
    add_child(ui)

The benefit here being that I can access the ui_instance from anywhere in the code, so I can also connect signals to it easily (although I actually haven't yet tested if it's better to connect to it through the singleton, as that will only get freed when exiting the game).

Phazorknight commented 2 months ago

Oh, this looks pretty good, shouldn't add too much complexity? I'm not too much of a fan off is the string-based path reference to the UI scene in the autoload/singleton though. I know they aren't always avoidable in Godot, but they always give me a stomach ache as they break as soon as you restructure your project a bit.

one thing to verify is that the scene management doesn't work any magic

Scene manager pretty much only moves the Player around and loads player data. I made the UI independent of this, so should be fine. 🤷🏼

The null check is there, because a scene change seems to also free the instance

Yeah, the level scene transition works by completely freeing the level scenes. In this case, having the PlayerHUD and PauseMenu dynamically added to the Level scene might make things a bit more complicated (though not impossible).

Just thinking out loud, I probably lean more towards adding the PauseMenu and PlayerHUD as "components" to the Player.tscn.

Pros:

Easier to understand for devs new to Cogito.
Easier to hook up signals/references between them.
Easier to set up new Cogito level scenes as only the player.tscn will need to be added.
Cogito Scene Manager keeps a reference to the currently active Player.tscn, so this can be also used to always have a reference to PlayerHUD and PauseMenu.
No need for another autoload/singleton

Cons:

Probably not the best practice as it counts as coupling
Potential layering issues with the Control nodes / UI elements. Though probably avoidable somehow (want to add functionality to hide/show crosshair etc.)

aronand commented 2 months ago

Oh, this looks pretty good, shouldn't add too much complexity?

Yeah, shouldn't add too much complexity. In this case, as long as a map/level is a CogitoScene the UI will get added to it during runtime. As it's done at the scene's _ready() function, a reference to the player can be passed to the UI as well if necessary, though this can be fetched from SceneManager as well if I remember correctly.

I'm not too much of a fan off is the string-based path reference to the UI scene in the autoload/singleton though [...] they break as soon as you restructure your project a bit.

True, true. For some hard coded strings I'm planning on adding them to a configuration file that can be edited through editor tools, and in case of UI I just hope I won't have to alter the structure too much 😄

Just thinking out loud, I probably lean more towards adding the PauseMenu and PlayerHUD as "components" to the Player.tscn.

That's definitely one way to do it and should work. For my own project I try to decouple things like that, so you don't have to access the player to get to the UI.

Quickly thinking about it, the only edge case where that could be an issue is a game that allows switching the controllable character (for an interesting example, see Battlefield 2: Modern Combat), but I guess that would be more of a game implementation detail in the end, as in how do you handle passing the control/updating the player object.

No need for another autoload/singleton

So I kind of ended up doing this in a hacky way because I like namespaces. I only have a single autoload, that acts as namespace for singletons + is config holder (global toggles etc.). This way I can then access every singleton from a single namespace.

extends Node
## A singleton that acts as a namespace for all other singletons.

# NOTE: An issue with this method of doing singletons, is that the autocompletion
# does not seem to function (at least in Godot 4.2.1)
var UserInterface: UserInterfaceSingleton = UserInterfaceSingleton.new()
var ItemRepository: ItemRepositorySingleton = ItemRepositorySingleton.new()
var Utils: UtilsSingleton = UtilsSingleton.new()

#region Paths
# TODO: Fetch the correct folder from a config instead
var items_path: String = "res://game/assets/items/"
#endregion

#region Global toggles
## Controls interactability globally.
var interactions_allowed: bool = true

## Controls whether interaction prompts are generated globally.
var generate_interaction_prompts: bool = true

## Controls whether carried items can be picked or not.
var can_pick_carried_item: bool = true
#endregion

If that were loaded simply as Cogito, you could use it like Cogito.UserInterface etc.

ac-arcana commented 2 months ago

I would like to add my two cents to this as I am working on an MP extension and I actively change the menu in my code rn.

Lots of these solutions sound good. The one thing I want to see is the ability to make non destructive changes on the users end. Basically all I'm say is that the dev using Cogito should be able to duplicate the menu, change it, and point to the new menu they made.

This is because if you change base cogito files and then want to update cogito you lose your changes when you do so.

On Thu, May 2, 2024, 01:58 aronand @.***> wrote:

Oh, this looks pretty good, shouldn't add too much complexity?

Yeah, shouldn't add too much complexity. In this case, as long as a map/level is a CogitoScene the UI will get added to it during runtime.

I'm not too much of a fan off is the string-based path reference to the UI scene in the autoload/singleton though [...] they break as soon as you restructure your project a bit.

True, true. For some hard coded strings I'm planning on adding them to a configuration file that can be edited through editor tools, and in case of UI I just hope I won't have to alter the structure too much 😄

Just thinking out loud, I probably lean more towards adding the PauseMenu and PlayerHUD as "components" to the Player.tscn.

That's definitely one way to do it and should work. For my own project I try to decouple things like that, so you don't have to access the player to get to the UI.

Quickly thinking about it, the only edge case where that could be an issue is a game that allows switching the controllable character (for an interesting example, see Battlefield 2: Modern Combat), but I guess that would be more of a game implementation detail in the end, as in how do you handle passing the control/updating the player object.

No need for another autoload/singleton

So I kind of ended up doing this in a hacky way because I like namespaces. I only have a single autoload, that acts as namespace for singletons + is config holder (global toggles etc.). This way I can then access every singleton from a single namespace.

extends Node## A singleton that acts as a namespace for all other singletons.

NOTE: An issue with this method of doing singletons, is that the autocompletion# does not seem to function (at least in Godot 4.2.1)var UserInterface: UserInterfaceSingleton = UserInterfaceSingleton.new()var ItemRepository: ItemRepositorySingleton = ItemRepositorySingleton.new()var Utils: UtilsSingleton = UtilsSingleton.new()

region Paths# TODO: Fetch the correct folder from a config insteadvar items_path: String = "res://game/assets/items/"#endregion

region Global toggles## Controls interactability globally.var interactions_allowed: bool = true

Controls whether interaction prompts are generated globally.var generate_interaction_prompts: bool = true

Controls whether carried items can be picked or not.var can_pick_carried_item: bool = true#endregion

If that were loaded simply as Cogito, you could use it like Cogito.UserInterface etc.

— Reply to this email directly, view it on GitHub https://github.com/Phazorknight/Cogito/issues/179#issuecomment-2089943583, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJVOI7SQW24H3FOCMYCKTTZAH54BAVCNFSM6AAAAABG2VEDPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBZHE2DGNJYGM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

aronand commented 2 months ago

If I understood correctly, essentially we'd want to have the ability to plug in your own UI scenes instead of using the built-in ones, right?

ac-arcana commented 2 months ago

Yes, that's what I would like to see.

On Thu, May 2, 2024, 08:51 aronand @.***> wrote:

If I understood correctly, essentially we'd want to have the ability to plug in your own UI scenes instead of using the built-in ones, right?

— Reply to this email directly, view it on GitHub https://github.com/Phazorknight/Cogito/issues/179#issuecomment-2090864794, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJVOI3VBJS5IMRP5O6CXP3ZAJOI7AVCNFSM6AAAAABG2VEDPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQHA3DINZZGQ . You are receiving this because you commented.Message ID: @.***>

aronand commented 2 months ago

Thinking about it, at least with the injection route I think some sort of a Cogito project config would be necessary to achieve that (not necessarily a config, could be some central script as well of course). As long as the custom UI implements a Cogito UI interface (so probably just create a base class which the UI script should extend), it should be good to use.

On the other hand, if attached to the player, the user could create a copy of the player scene, modify it, and use that in their own levels. Might cause some headaches if trying to integrate changes from Cogito to own project, depending on how heavily it otherwise stays dependent on Cogito's scripts, or doesn't.

ac-arcana commented 2 months ago

Currently in my mp project I've extended the menu class, so either approach seems workable to me.

To add some context, I've got a MP extension scene which loads the demo scene in and then finds the menu, deletes it, and replaces it with a new one.

On Thu, May 2, 2024, 10:37 aronand @.***> wrote:

Thinking about it, at least with the injection route I think some sort of a Cogito project config would be necessary to achieve that. As long as the custom UI implements a Cogito UI interface (so probably just create a base class which the UI script should extend), it should be good to use.

On the other hand, if attached to the player, the user could create a copy of the player scene, modify it, and use that in their own levels. Might cause some headaches if trying to integrate changes from Cogito to own project, depending on how heavily it otherwise stays dependent on Cogito's scripts, or doesn't.

— Reply to this email directly, view it on GitHub https://github.com/Phazorknight/Cogito/issues/179#issuecomment-2091140767, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJVOI5TITNPZNMIFAAXHR3ZAJ2WPAVCNFSM6AAAAABG2VEDPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJRGE2DANZWG4 . You are receiving this because you commented.Message ID: @.***>

Phazorknight commented 1 month ago

Sorry for the late reply, had to deal with my main game project and then worked on some other ends of Cogito.

@aronand: some sort of a Cogito project config would be necessary

I was thinking this more and more recently. In Unity there's the Indexer pattern, that can be used to specify a global singleton that works like a ScriptableObject. I'd love to have something similar in Cogito, though we'd probably have to go the plug-in route of creating a Cogito tab at the top bar to get that level of comfort and usability. While it might be a bit of work, it could offer an opportunity to create a central place for some global Cogito project settings, especially down the line (like a manager for resources like CogitoItems).

For the 1.0 release I'd probably keep it simpler though.

ac-arcana commented 1 month ago

What about a resource for Cogito configuration? It could be top level in the cogito directory and we can point it out in the readme. In Unity terms its like a Scriptable Object

I think I would be happy though with an object in the scene that you point to a player scene which already contains a player, hud, and pause menu. Users could just change the scene it is pointed to. This way all the correct player settings are loaded for their project rather than needing to make sure it is set up correctly in every single scene. Perhaps this could be handled in scene loading.

Phazorknight commented 1 month ago

So I've just pushed an update that adds the Player HUD and Pause Menu to the Player.tscn in https://github.com/Phazorknight/Cogito/commit/64c020ddc3780780f8b8928cf66fb94266d3bb63

Please take a loo and see if this makes things a bit easier.

@aronand I've opted for this route for now just for simplicity, feel free to keep your PR #180 open for further experimentation with this.

Phazorknight commented 1 month ago

Closing this now as it's no longer an issue per se.

Phazorknight / Cogito

Dynamically add player HUD to map scenes #179

Singleton pseudocode

region Paths# TODO: Fetch the correct folder from a config insteadvar items_path: String = "res://game/assets/items/"#endregion

region Global toggles## Controls interactability globally.var interactions_allowed: bool = true

Controls whether interaction prompts are generated globally.var generate_interaction_prompts: bool = true

Controls whether carried items can be picked or not.var can_pick_carried_item: bool = true#endregion