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

Allow customisation of channel used for GPS #1776

Open
djmattyg007 opened this issue Apr 7, 2024 · 5 comments
Open

Allow customisation of channel used for GPS #1776

djmattyg007 opened this issue Apr 7, 2024 · 5 comments
Labels
area-CraftOS This affects CraftOS, or any other Lua portions of the mod. enhancement An extension of a feature or a new feature.

Comments

@djmattyg007
Copy link

For a few years now, GPS has all run over a single channel. This is totally fine if you only use GPS for truly global positioning following Minecraft's coordinates system. However, it's explicitly documented that you don't have to do this. You can set up a GPS network with its own coordinates system, and as long as everything participating in the network agrees on what the coordinates mean, you should be fine.

I'm currently playing a skyblock pack (FTB Skies to be precise). My world layout involves creating lots of interconnected 15x15 platforms, each of which is used to perform a specific task. In my case, I'm setting up a tree farm managed by a ComputerCraft turtle. To make sure that the turtle always knows where it is, I have a GPS network set up. To simplify the programming, I've localised this GPS network to this platform specifically. A host on one corner of the platform is registered as x:0,y:0,z:0. A host on the opposite corner is registered as x:14,y:6,z:14. This makes programming the turtle significantly easier, because I don't need any kind of translation layer between Minecraft's global coordinates and my platform specifically.

The problem comes when I try to set up another platform nearby with a similar configuration. All of these hosts are operating on the same channel, and there's nothing that stops hosts for one platform from responding to queries for another platform. This would provide risk providing wildly inaccurate responses to anything that needs it.

I believe there's a simple solution to this problem - allow customisation of the channel a program uses for GPS. I've tested this by creating my own "gps2" package, and it seems to work perfectly. As long as everything is using an agreed-upon channel, there don't seem to be any conflicts. If there's interest I'm happy to create a PR, however I don't think it would be easy to retrofit into the existing code.

@djmattyg007 djmattyg007 added the enhancement An extension of a feature or a new feature. label Apr 7, 2024
@djmattyg007
Copy link
Author

I read somewhere that APIs are deprecated and modules is the future. This makes sense to me - when I tried to require cc.pretty in my gps2 API, it failed spectacularly, preventing computers from booting. I couldn't even reboot them to fix it, I had to exit the game and re-open it :(

Perhaps the creation of "gps2" could help make that transition for the GPS code.

@SquidDev SquidDev added area-Minecraft This affects CC's Minecraft-specific content. area-CraftOS This affects CraftOS, or any other Lua portions of the mod. and removed area-Minecraft This affects CC's Minecraft-specific content. labels Apr 7, 2024
@SquidDev
Copy link
Member

SquidDev commented Apr 7, 2024

So require (much like the shell API) is only available when running a program. Each program gets its own copy of require (and thus the package table), which allows programs to be isolated from each other.

However, APIs are loaded into the global environment, which means that require is not available. You can add it, but normally it's just better to create a module rather than an API.

I couldn't even reboot them to fix it, I had to exit the game and re-open it :(

Hrmr, that's a little concerning. Was this in a datapack — in this case I assume you were hitting this error? It not, would you be able to file a bug with steps to reproduce — Lua errors definitely shouldn't be bricking computers!

@Wojbie
Copy link
Contributor

Wojbie commented Apr 7, 2024

I believe there's a simple solution to this problem - allow customisation of the channel a program uses for GPS.

You could also achieve same effect by having computers on each island carry modified gps api on their drives with different channel set to be used and unload rom gps api then load modified one from harddrive. Is this feature worth making gps api more complicated?

@djmattyg007
Copy link
Author

Hrmr, that's a little concerning. Was this in a datapack — in this case I assume you were hitting this error? It not, would you be able to file a bug with steps to reproduce — Lua errors definitely shouldn't be bricking computers!

The relevant file was in a datapack, yeah. I can file a (separate) bug report for the bricked computers. I'll need to double-check what error message I was seeing, but I do remember that it was truncated, making the issue difficult to track down.

However I am conscious that I'm not on the latest version of ComputerCraft, so I'm not sure what kind of support it gets. I'm on the latest version for Mnecraft 1.19.2 though.

You could also achieve same effect by having computers on each island carry modified gps api on their drives with different channel set to be used and unload rom gps api then load modified one from harddrive. Is this feature worth making gps api more complicated?

This would involve copy+pasting a lot of code around. If I wanted to make a backwards-compatible improvement to the GPS code, I would then have to manually update all of the code on all of my computers. That doesn't sound like a nice solution to me.

There are other improvements that I've made to the GPS code in my (currently unpublished) "gps2" code:

  • The hosting code has been moved into a module too.
  • The hosting code accepts an "observer" function that fires whenever a GPS request is successfully served, so that the hoster can do whatever they want when that happens, not just forcibly print things to the screen.
  • The client code returns the vector object directly, rather than separate integers as a tuple.
  • The code uses vector objects a lot internally, and they expose a tostring method, so I simplified a lot of the debugging print statements.

Other improvements I want to make (but haven't yet) include improving how error handling happens (kind of like what's suggested in #1771), and exposing some more of the internals of the code so that not as much of it has to be copy+pasted in case someone wants to customise it further.

I'm not really sure how most of these improvements can be made in a backwards-compatible manner. When you also look at #1627, it starts me thinking that we really should just have a "gps2", and gradually deprecate "gps1" (gps as a global API).

To be clear, I'm not suggesting that specifying a channel should be compulsory. It should absolutely be possible to continue using a default GPS channel and not have to care about any of this. The gps2 CLI program I wrote to wrap the new module code uses a default channel, configured through the settings system.

The module code doesn't currently use this default, but perhaps it doesn't need to. Perhaps it's acceptable that when using the gps2 module you have to specify a channel, and gps is rewritten as a simple thing module that wraps around gps2, but provides the old API (so that users only have to add a require() call and keep moving).

@SquidDev
Copy link
Member

SquidDev commented Apr 9, 2024

I'll need to double-check what error message I was seeing, but I do remember that it was truncated, making the issue difficult to track down.

Oh, that's really weird — all the printing functions there should be word-wrapping. I don't think that code has changed much since 1.19.2, so if you're happy to have a file a bug, that would be super useful.

Reporting for startup errors is all a bit rubbish right now — it's not something most people hit, and so easy to forget about. I'm sure there's some improvements to be made here, will have a think.


With regards to #1627, backwards compatibility isn't a big concern. The interface shouldn't need to change at all, and the protocol changes can be done as an optional extension. The main change needed in that PR is that we need a unique ID per request in order to distinguish between them, and I don't think Matthew or I have had a chance to look at that yet.


I'd definitely be open to adding some gps.host function! There's a whole bunch of extra functionality I'd quite like to support (multiple modems, broadcast-only mode), that are too complex to expose via a CLI, so definitely require some programmatic interface.

My gut feeling is that, aside from the changes to return value, most of this could be added to the existing GPS API without breaking back compat. Is there anything you foresee difficulties with?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CraftOS This affects CraftOS, or any other Lua portions of the mod. enhancement An extension of a feature or a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants