-
Notifications
You must be signed in to change notification settings - Fork 212
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
WIP: Modem peripherals toggling #510
base: mc-1.15.x
Are you sure you want to change the base?
Conversation
Looks like #264 is related. |
Regarding API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModemPeripheral
/ModemState
does weird things with TickScheduler
because we don't actually care about what's done on the main thread - it's only there to update the visual state.
In our case, we do probably care about the result of togglePeripheralAccess
- it'd be nice if we returned whether peripherals were enabled or not (or something). In this case, we don't need to do anything with AtomicBoolean
s or anything - just add a @LuaFunction( mainThread = true )
on togglePeripherals
and you should be safe to call the TE method directly.
As far as naming goes, I'm not too sure. Definitely agree that either is
/set
or is
/enable
/disable
is the way to go. Not really sure what the best name is though - "Active" would be one candidate I guess, but it's not super clear.
@@ -61,4 +61,9 @@ public void networkChanged( @Nonnull IWiredNetworkChange change ) | |||
protected abstract void attachPeripheral( String name, IPeripheral peripheral ); | |||
|
|||
protected abstract void detachPeripheral( String name ); | |||
|
|||
public void togglePeripheralAccess() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth making this abstract instead, though I realise this may just be a placeholder while we work through the design :).
Interesting thing happens. With full block modems everything works as expected, but with cable modems it's only possible to share a computer directly attached to the modem. Turtles don't see cable modems as peripherals at all. Therefore they can't call any methods. Probably some |
Wired modems shouldn't be able to attach to turtles at all, as turtles aren't a full block, so I don't really think this is an issue. |
I agree, turtles shouldn't use cable modems as modems. But they should be able to activate/deactivate neighbor modem as part of construction process. This could be a new type of peripheral, or may be possibility to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, that makes sense. I'd not worry about that right now - adding another peripheral is kinda gross, and if people really want it they can use full block modems.
{ | ||
if( sharingPeripherals == enabled ) return sharingPeripherals; | ||
sharingPeripherals = enabled; | ||
if( updateTile ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this as a boolean flag feels super janky. It's probably worth having two methods, where one of them is a trySetSharingPeripherals
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Now it's two methods.
Alternatively, |
if this is WIP, shouldn't this be a draft PR? |
Thinking of picking this up, am I right in thinking this issue in the mod was never resolved? |
correct. no such feature exists in the main mod. |
In #1469,
I suppose this PR can be closed too. |
This PR adds peripheral methods to the modem themselves, not the turtle. This is mostly stalled as I'm still not 100% convinced on the idea, but not against it either. |
Maybe turtles could toggle modems by using |
The inability of a turtle to turn on a modem feels more like something is missing than anything else. I actually went searching in the modem API to find the functions that this PR would add because I assumed it would be present, given that this is a computercraft peripheral and not a random block interaction. But I also feel like the arbitrary limiting of interactions is not great in the first place. Rather than hard-coding what a turtle can and can't interact with, why not make it a configurable option? I'd much rather be able to go into my server's config folder and simply enable the feature, or as a modpack dev include a default configuration for it based of the balance that I'm trying to achieve, much like how you can now with fuel usage. |
I would like to toggle modems by turtles to automate building.
Regarding the code: it works. Only with fullsized wired modems.
There's nothing else which could be called good, since I'm complete newbie in modding, in CC codebase and in Java to complete the picture. Therefore I'm ready to be kicked for bad code/patterns/structure.
I'm planning to add similar thing to usual wired modems and to think more about Lua API. And of course I'm open for discussion.