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

WIP: Modem peripherals toggling #510

Open
wants to merge 1 commit into
base: mc-1.15.x
Choose a base branch
from

Conversation

neumond
Copy link
Contributor

@neumond neumond commented Jul 27, 2020

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.

@neumond neumond marked this pull request as draft July 27, 2020 11:20
@neumond
Copy link
Contributor Author

neumond commented Jul 27, 2020

Looks like #264 is related.

@neumond
Copy link
Contributor Author

neumond commented Jul 27, 2020

Regarding API.
Obviously I don't like current modem.togglePeripherals() method. This should be is... + set... pair. But I can't pick good name for this.
Another issue is that peripherals can't be detected from coroutine thread (when turning on) and we can't simply return whether it will be enabled or not. I looked at how modem.open updates the block, but it seems to be one-way unconditional passing of boolean with subsequent TickScheduler.schedule, then the server thread takes new value and updates the block.

@SquidDev SquidDev added area-Minecraft This affects CC's Minecraft-specific content. enhancement An extension of a feature or a new feature. labels Jul 27, 2020
Copy link
Member

@SquidDev SquidDev left a 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 AtomicBooleans 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()
Copy link
Member

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 :).

@neumond
Copy link
Contributor Author

neumond commented Aug 10, 2020

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 modem_control lightweight peripheral can solve the issue, but I would like to discuss this.

@SquidDev
Copy link
Member

Turtles don't see cable modems as peripherals at all

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.

@neumond
Copy link
Contributor Author

neumond commented Aug 10, 2020

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 place() (right click) at the side of modem. Probably better alternative exists.

Copy link
Member

@SquidDev SquidDev left a 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 )
Copy link
Member

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.

Copy link
Contributor Author

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.

@Ocawesome101
Copy link
Contributor

isModemActive and setModemActive could be good names.

Alternatively, isPeripheralActive and setPeripheralActive, or isActive and setActive, work if you don't want to tie this directly to the modem peripheral.

@ghost
Copy link

ghost commented Oct 25, 2021

if this is WIP, shouldn't this be a draft PR?

@merlinlikethewizard
Copy link

Thinking of picking this up, am I right in thinking this issue in the mod was never resolved?

@Ocawesome101
Copy link
Contributor

correct. no such feature exists in the main mod.

@khankul
Copy link
Contributor

khankul commented Oct 29, 2023

In #1469,

While I understand the use-case, it feels strange to me to add a method to the turtle API for interacting with a single block.

I suppose this PR can be closed too.

@SquidDev
Copy link
Member

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.

@powerboat9
Copy link
Contributor

Maybe turtles could toggle modems by using turtle.place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Minecraft This affects CC's Minecraft-specific content. enhancement An extension of a feature or a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants