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

feat: add esc_current_scene reserved global #474

Merged
merged 3 commits into from
Dec 3, 2021

Conversation

BHSDuncan
Copy link
Collaborator

New reserved global to hold current scene global ID to allow for flexibility in ESC scripts.
Also refactor reserved globals' handling for rooms with addition of ESCRoomManager and the elimination of string literals across multiple files. Allows for less chance of accidental errors as opposed to using string literals everywhere. As well, this encapsulates room-specific reserved globals and will serve as the basis for more room-specific behaviour management.

…s' handling for rooms with addition of ESCRoomManager and elimination of string literals across multiple files
@@ -0,0 +1,32 @@
extends Object
class_name ESCRoomManager
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not so fond of the name of this class: ESCRoomManager seems to imply this class manages rooms, but this is not true as it's just an enum entries holder. Would ESCReservedGlobals fit instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, my idea was that this manager would also change the room for example. Also, why's the GLOBAL_ANIMATION_RESOURCES here? That doesn't have to do anything with rooms if I'm not mistaken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. You saw it in ESCRoom and put it here. It's actually the global cache of the current animation resource of the player, so it doesn't really fit here. Hm... maybe ESCPlayer? I'm not 100% sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, my idea was that this manager would also change the room for example

Can you give more precisions on this?

Copy link
Collaborator Author

@BHSDuncan BHSDuncan Dec 1, 2021

Choose a reason for hiding this comment

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

Ah, I see. You saw it in ESCRoom and put it here. It's actually the global cache of the current animation resource of the player, so it doesn't really fit here. Hm... maybe ESCPlayer? I'm not 100% sure.

Yeah, you got it. It's also only ever used in ESCRoom, it seems. But I'm happy to put it wherever you deem best.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it also used in the set_animations command? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, good catch. I didn't see it because it's actually referenced by a one-off constant in globals_manager, so I'll need to fix that anyway.

But like I said: I'll put the definition in whichever class you prefer!

addons/escoria-core/game/escoria.gd Show resolved Hide resolved
Copy link
Collaborator

@dploeger dploeger left a comment

Choose a reason for hiding this comment

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

Good first PR. :) Some remarks though.

)
_reserved_globals[key] = value
_globals[key] = value
emit_signal("global_changed", key, _globals[key], value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say, you should only emit the signal, if the value is not null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

@@ -0,0 +1,32 @@
extends Object
class_name ESCRoomManager
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, my idea was that this manager would also change the room for example. Also, why's the GLOBAL_ANIMATION_RESOURCES here? That doesn't have to do anything with rooms if I'm not mistaken.

@@ -0,0 +1,32 @@
extends Object
class_name ESCRoomManager
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. You saw it in ESCRoom and put it here. It's actually the global cache of the current animation resource of the player, so it doesn't really fit here. Hm... maybe ESCPlayer? I'm not 100% sure.

@@ -54,7 +54,9 @@ func _on_button_pressed():
# automatic transitions.
# If FORCE_LAST_SCENE_NULL is True when change_scene starts:
# - ESC_LAST_SCENE is set to empty
escoria.globals_manager.set_global("FORCE_LAST_SCENE_NULL", true, true)
escoria.globals_manager.set_global( \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd better format that like

escoria.globals_manager.set_global(
  escoria.room_manager.GLOBAL_FORCE_LAST_SCENE_NULL,
  true,
  true
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, you don't need the \ in the first line. Also, please put the closing bracket into the next line all to the left aligned with the command itself.

@BHSDuncan BHSDuncan requested review from dploeger and StraToN December 2, 2021 17:38
Copy link
Collaborator

@dploeger dploeger left a comment

Choose a reason for hiding this comment

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

Ah, I guess you still need to fill ESCRoomManager with the business logic of change scene and everything that is not related to just one room from ESCRoom and ESCChangeSceneCommand

@@ -54,7 +54,9 @@ func _on_button_pressed():
# automatic transitions.
# If FORCE_LAST_SCENE_NULL is True when change_scene starts:
# - ESC_LAST_SCENE is set to empty
escoria.globals_manager.set_global("FORCE_LAST_SCENE_NULL", true, true)
escoria.globals_manager.set_global( \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, you don't need the \ in the first line. Also, please put the closing bracket into the next line all to the left aligned with the command itself.

@BHSDuncan BHSDuncan requested a review from dploeger December 2, 2021 18:25
Copy link
Collaborator

@dploeger dploeger left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks. I'll approve this like we talked on Discord. Need to wait for Julian's comment, though.

@dploeger
Copy link
Collaborator

dploeger commented Dec 2, 2021

Oh, I forgot: Great work. 🎁 :)

@BHSDuncan
Copy link
Collaborator Author

Oh, I forgot: Great work. 🎁 :)

Thank you! Hopefully I can always produce good work!

@StraToN
Copy link
Collaborator

StraToN commented Dec 3, 2021

All good for me. Good remarks and great addition, great work! Thanks a lot!

@StraToN StraToN merged commit 391bf08 into godot-escoria:develop Dec 3, 2021
balloonpopper pushed a commit to balloonpopper/escoria-demo-game that referenced this pull request Feb 13, 2022
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.

3 participants