fix: prevents duplicate item registration and also ensures that the p…

[BHSDuncan]

…layer is forcibly registered

Fixes godot-escoria/escoria-issues#203

[balloonpopper]

If 2 ESCItems have the same global ID this fails correctly. I've found 2 issues though :

1. I create 2 ESCItems (ESCItem1 and ESCItem2). I create an ESCLocation as a child of ESCItem2 and give it the same Global ID as ESCItem1. Running the scene does not cause it to crash.

2. The error message "Object with global id button_main_change_scene in room (room14, 1931) already registered" tells me that the IDs should be globally unique. If I have ESCItem1 in room 1 with the ID ESCItem1_ID and create an object in room 2 with the same ID ("ESCItem1_ID") it doesn't crash as I go back and forth between room 1 and 2.

[BHSDuncan]

1. ESCLocation also forces registration. @StraToN I'm assuming there's a reason for this but maybe you can answer authoritatively?

2. I honestly don't know what to do about this...I know you're not saying it's the case, but the object manager's responsibility isn't to police global uniqueness (at least not anymore, although previously it only checked during registration and not beforehand). Maybe we should just change the error message to say, "Object with id ..." (removing the "global" part) if it's that confusing. To me, I don't read it the same way; to me, it sounds like that global ID has already been registered with that room and can't be registered again. It doesn't suggest anything about being globally unique. But again, that's just my take on it.

So we can discuss that error message, as well as if/how we want to police global ID uniqueness, if at all.

@StraToN you may want to weigh in on this as well.

[StraToN]

@BHSDuncan @balloonpopper

Indeed there is no strong rule about objects' global_id 's uniqueness in the global project. However, you should not be able to have 2 objects with the same global_id in the same room.
Therefore it is normal we have no crash for having 2 different objects sharing the same global_id in 2 different rooms because they're registered in the Object Manager under the room key they belong to.

The downside of this is that you cannot differentiate the 2 items when calling an ESC command on that global_id. In that case, the Object Manager will return current room's object that has this global_id (if any), else it will return the first object with that global_id it can find.

[balloonpopper]

Is there any reason we can't make them globally unique? If something with the name "global" it's not going to be a globally unique identifier then to me it's like if you were programming using a "const" and changed its value.

(The alternative would be renaming it "object_id" but I think that's a bigger task than anyone wants to take on :D )

If the decision stays that you can have a global_id that isn't "global", then can this PR include documentation updates to clarify that please.

There is also still this issue - "I create 2 ESCItems (ESCItem1 and ESCItem2). I create an ESCLocation as a child of ESCItem2 and give it the same Global ID as ESCItem1. Running the scene does not cause it to crash." @StraToN, I believe this is waiting on your input ("ESCLocation also forces registration. @StraToN I'm assuming there's a reason for this but maybe you can answer authoritatively?")

[StraToN]

Is there any reason we can't make them globally unique? If something with the name "global" it's not going to be a globally unique identifier then to me it's like if you were programming using a "const" and changed its value.

Objects Manager "knows" about objects that were already met while playing. Escoria has no way to know every ESCItem defined in the game because these are defined in different scenes, which Escoria cannot monitor.

We could forbid having 2 objects globally sharing the same global_id, but at the moment Escoria meets this it is already too late. If this happens, then either Escoria crashes or Escoria warns it in the logs and manages both objects based on the pair <room_id, object_global_id>. Which is what is currently being done, if I'm not wrong?

There is also still this issue - "I create 2 ESCItems (ESCItem1 and ESCItem2). I create an ESCLocation as a child of ESCItem2 and give it the same Global ID as ESCItem1. Running the scene does not cause it to crash." @StraToN, I believe this is waiting on your input ("ESCLocation also forces registration. @StraToN I'm assuming there's a reason for this but maybe you can answer authoritatively?")

This should not happen in that case. 2 objects in the same room should not share the same global_id, even if both are not the same type and force registration. You can make a character walk towards an ESCItem or towards an ESCLocation, both walk cases are treated the same.

Therefore I'd just stick to the rule: having 2 objects sharing the same global_id in the same room is an error.

---

Also just fyi: ""global_id" is just a historic name from old Escoria. We just didn't change it.

[balloonpopper]

Thanks @StraToN. We can make the documentation say that the global_id is unique to each scene and not to the whole game. If anyone's looking for me, I will be sitting here grumbling to myself about how I disagree with the poor naming of "global" IDs 😠 😆

We still have a bug therefore. I've created an ESCLocation in room 3 and given it the same Global ID as the left door exit, and it doesn't break Escoria.

[BHSDuncan]

@balloonpopper the bug makes sense since ESCLocation has "force registration" set to true.

@StraToN do we need to force registration for ESCLocation?

[StraToN]

@balloonpopper the bug makes sense since ESCLocation has "force registration" set to true.

@StraToN do we need to force registration for ESCLocation?

I think we can safely remove the "force registration" in ESCLocation. Just ensure there is a warning pushed in the logger so that gamedev knows that the global_id is duplicated.

Thanks @StraToN. We can make the documentation say that the global_id is unique to each scene and not to the whole game. If anyone's looking for me, I will be sitting here grumbling to myself about how I disagree with the poor naming of "global" IDs angry laughing

I am not married to "global_id" :) (especially since I didn't set this name myself in the first time).
This parameter name has no real impact on Escoria if it was to be renamed. Simply all documentation has to follow the change, that's all.
IMO, gamedevs don't care what this parameter is named, be it "global_id" or "local_id" or anything else, so I have no objection if you want to set it a better name. This change would just be better for Escoria devs, more than for gamedevs ;)

[balloonpopper]

The global_id name isn't urgent, we can worry about that another time.
I'm not sure what I can test about the force_registration changes. I assume that was the error messages
"Object with global ID player room (room2, 4189) not found. If this was
part of a 'forced' registration, ignore this warning."
It's great to see them all gone!

I also can't create a child ESCLocation with the same name as another ESC Item now, so that's been fixed.

Is there anything else that needs to be tested?