Add lock_state_changed signal to CogitoDoor

[ac-arcana]

Just adding and emitting a signal when the door's lock state is changed. I used this in order to sync door locks in my Coop project, but it could be used to hook anything to a door being unlocked.

[aronand]

One tiny change could be to add a setter for is_locked, which handles emitting the signal whenever the state changes. This way no matter what causes the state to change (e.g. a lock_door() method is introduced later down the line, or someone manually sets the state somewhere else), the signal gets emitted.

This would look something like:

is_locked: bool:
    set = _set_is_locked

# ...

func _set_is_locked(value: bool) -> void:
    is_locked = value
    lock_state_changed.emit(is_locked)

We might also want to first verify that the state is actually different before emitting anything.

[ac-arcana]

I'm still learning gdscript. Is defining the variable like that going to call the function whenever the variable is changed, regardless of how it is changed?

I wasn't aware of setters like this in gdscript.

[aronand]

Yup, whenever the value is set it calls the function defined as the setter with said value (and you can also use get for getting values, I added some in #173). You can either define it directly under the variable, or give it a function that it will call.

Here's the documentation page on them: https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_basics.html#properties-setters-and-getters

Personally I prefer defining setters and getters as their own functions if there's more logic in them, so the variable declarations at the top remain cleaner and more readable. But, there's also the case to be made for locality of information. It's easier to see what a setter does when the code is right there 😄

[ac-arcana]

Thanks for linking the docs. I see value in a setter and getter, but it also begs the question if the same should be done for is_open. Doing it for both it doesn't really increase functionality for games. I propose instead that I write a simple lock_door function. This would actually increase functionality for games and cover all use cases for locks.

[aronand]

True, is_open could have one too. It's really just a way to ensure that something happens when the state changes, even if the state is changed in the "wrong" way (e.g. for whatever reason directly altering the variable instead of using a method in some script). This way if you also do end up having more methods altering the state and sending signals, you keep the code a bit more DRY.

Alternatively, the public is_locked could be a getter only that returns the state of a private _is_locked variable, which encourages use of the intended methods.

EDIT: Also, if using a private internal value which has a public getter and setter, you can use the setter to call the correct method for the caller, something like:

var _is_locked := false
var is_locked: bool:
    get: return _is_locked
    set(value):
        if value == true:
            lock_door() # sets the private _is_locked to true
        else:
            unlock_door() # sets the private _is_locked to false

But these are just a suggestion of course, the PR as is fits the existing codebase just fine.

[ac-arcana]

The way you put DRY in all caps makes me think this is a programming concept I haven't heard of, too. Lol

On Tue, Apr 23, 2024, 22:32 aronand @.***> wrote:

True, is_open could have one too. It's really just a way to ensure that something happens when the state changes, even if the state is changed in the "wrong" way (e.g. for whatever reason directly altering the variable instead of using a method in some script). This way if you also do end up having more methods altering the state and sending signals, you keep the code a bit more DRY.

— Reply to this email directly, view it on GitHub https://github.com/Phazorknight/Cogito/pull/177#issuecomment-2074064957, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJVOIZZVJ3CHQOGH26YI23Y647YVAVCNFSM6AAAAABGT5ZEZGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZUGA3DIOJVG4 . You are receiving this because you authored the thread.Message ID: @.***>

[aronand]

Haha, yeah, it is 😄 DRY = Don't Repeat Yourself

[ac-arcana]

I think I'm going to leave this pull request as is.

@aronand I agree with the suggestions you are making, but in practice it's a little finnicky. I feel like keeping open_door, close_door, and unlock_door public and the is_open and is_locked variables "private" would be the proper way to do this. However, the variables are used initially to set the state for the start of the scene so they aren't entirely "private"....

Trying to do it the other way, where I use setters and getters I run into a recursion problem. The interact function is already calling functions like open_door, close_door, and unlock_door. This could be rewritten to set the variable instead, but then there is the problem of passing the interactor to the open_door function.

I just don't feel like any solution I have come up with in practice is cleaner or better, there is always something still wrong with it. If someone else wants to improve this further, feel free.

[aronand]

Yeah the is_open and is_locked flags are something that are good to be publicly readable at least, as someone might want to check those in a script of their own.

I can have a go at a possible implementation during the week when I have the time, but I think it can also be turned into an issue of its own, especially as it's looking to be a larger refactor than just a small edit in logic in a single file, and is not critical for the functionality of your changes.

EDIT: Right, ran a quick test on it this morning and yeah, doing these changes would seemingly need to refactor scene management as well. It looks like everything works as expected when initially loading in, but if you move to a different scene and then come back, the is_locked flips on all doors, so unlocked doors become locked and (presumably) vice versa. I actually discovered a potential bug as well, as some doors that I left open had now vanished from the map, need to see if it happens in main as well.

EDIT2: Oh alright there might be a bug in scene management actually. In main, opened doors (is_open == true) are either visually closed, in wrong positions, or have completely vanished (or most likely moved to a really weird location in the scene). See #181