Grocel / 3D-Stream-Radio

It is an spawnable scripted entity for Garry's Mod that allows you to play online streams and local files as 3D world sound.
https://steamcommunity.com/sharedfiles/filedetails/?id=246756300
17 stars 4 forks source link

IsConnectedWirelink does not check for "virtual" wirelinks #20

Closed Noxyro closed 8 months ago

Noxyro commented 11 months ago

Issue:

When connecting to a Stream Radio entity with a "virtual" wirelink^1, the IsConnectedWirelink function does not recognize that wirelink, because it only checks if the "physical" input has been wired to something.

See IsConnectedWirelink function: https://github.com/Grocel/3D-Stream-Radio/blame/6fa509874358b5bd58c6ba87fb1e6ee6e55866a1/lua/entities/base_streamradio.lua#L812-L814

Result:

When trying to set / trigger an input change of (for example) the Stream URL (does apply to any other input as well), it will not actually change the internal value due to the hard-coded check if that specific input has been wired.

See here: https://github.com/Grocel/3D-Stream-Radio/blame/6fa509874358b5bd58c6ba87fb1e6ee6e55866a1/lua/entities/sent_streamradio/init.lua#L983C3-L985


This also leads to the following effects:

1. Inconsistency: the majority of other wireable entities do not ignore "virtual" wirelinks and their input changes, even if the corresponding "phyiscal" wire input has not been actually wired.

2. Confusion: it is not visible or known to someone working with the entity, that inputs will be "scrapped" if the corresponding wire input is not "physically" hooked up to something.

For example (Expression 2): Wirelink = entity():isWeldedTo():wirelink() - returns the wirelink of that entity the chip is welded to, without actually ever wiring the input of that entity

Grocel commented 11 months ago

Thanks for your well written report. I like how in depth it is. This is indeed a rare sight. šŸ‘

This is actually a known issue. The check is needed to prevent other bad behaviors and race conditions. It is hard to do proper prioritization of data when other inputs such as tool or user inputs via GUI are present. The radio needs to differentiate if an input was just "empty" or actually not used at all. So removing the check is not an option.

However the IsConnectedWirelink is incomplete and I would like to adjust it to account for the virtual wirelink as well. I don't know how to check for the virtual wirelink being preset, though. I am not sure if Wiremod delivers the needed information it its API. Do you have any tips for me on that?

For the time being I would recommend to use a phyiscal wirelink you can create it on the entity on the fly using the wire tool.

Grocel commented 11 months ago

It should be much better now. It is not perfect, but virtual wirelinks do work now. A known problem is once a Wirelink as been created on a radio by e2 or via the tool, the check for being wired is effectively always return true. Even if the Link/E2 has been removed. This can create some odd behavior on some cases when reverting/removing inputs.

Noxyro commented 11 months ago

Thanks for implementing this šŸ‘ Honestly, this was an edge-case scenario that probably only very few people (like me šŸ˜„) ever stumbled into, so thank you for taking the time to fix it (this quickly) anyways!

I thought about what you said regarding the odd behaviour and if I haven't missed anything, this should not be a problem. After removing / reverting the wirelink, the following can happen:

  1. No wirelink connected and no other inputs are wired, means none of the inputs (should) receive any input triggers - since the "wired" check is only relevant on input triggers, this is perfectly fine.
  2. No wirelink connected but other inputs are wired, means that only the wired inputs can receive any input triggers - since the or-statement for "wired" in the TriggerInput function checks the physical inputs first anyways, this is also perfectly fine.
  3. Wirelink reconnected and no other inputs are wired, means input triggers can only come through the wirelink (physical or virtual, does not matter because of the fix now) and its basically the same state as before removal, so also totally fine.
  4. Wirelink reconnected and other inputs are wired, means the wirelink will make the checks always true (already did before the fix), which is required for making wirelinks work as they do in the current setup, so this is also fine.

As you can see, none of the above cases should give reason to worry and in my opinion this behaviour is totally acceptable. But of course I could have missed something, so correct me if I'm wrong šŸ˜ƒ

Grocel commented 11 months ago

The issue is that there is no good way to check for the wirelink being connected that also respects the virtual wirelink. It is an issue with the Wiremod API. So when a wirelink has been created, it can be never be removed/unwired again. This didn't happen before the fix. This leads the iswired check to always be true no matter what once the wirelink has been touched.

While this might not be a problem for most cases, it is for things like "Time", "Volume" or "Radius". These have non-zero defaults. The wired check is for disconnections reverting to default instead of zero. Issues could also arise in remembering/reapplying pre syncing states after unwiring the "Master Radio", etc.

In hindsight, I also think that those issues are still totally acceptable, given the advantages of virtual wirelinks. This can be seen as a good trade off of a bug vs. another bug.

Grocel commented 9 months ago

@Noxyro: Recent changes to Wiremod might brake this feature again. There are no alternatives known for properly checking for e2 wirelinks yet. The relevant change: https://github.com/wiremod/wire/pull/2891

Reopening this issue for further development.

Edit: There is an upcoming change request that will help to fix this issue. https://github.com/wiremod/wire/pull/2942

Grocel commented 8 months ago

Refixed with latest update. Should work with old and new Wiremod versions.