When the terminal data is not present, width/height are set to 0, rather
than the terminal's width/height. This meant we'd create an empty
terminal, which then crashes when we try to render it.
We now make the terminal nullable and initialise it the first time we
receive the terminal data. To prevent future mistakes, we hide
width/height, and use TerminalState.create everywhere.
Fixes#1765
I have mixed feelings about speaker.playSound. On one hand, it's pretty
useful to be able to play any sound. On the other, it sometimes feels
... maybe a little too magic?
One particular thing I don't like is that it allows you to play
arbitrary records, which sidesteps both a vanilla mechanic (finding
record discs) and existing CC functionality (disk.playAudio). We now
prevent playing record tracks from the speaker.
In 5d8c46c7e61117ede5d6315efc81a4ae498f34ba, we switched to using UUIDs
for looking up computers (rather than an integer ID). However, for
compatibility in some of the command code, we need to maintain the old
integer lookup map.
Most of the code was updated to handle this, *except* the code to remove
a computer from the registry. This meant that we'd fail to remove a
computer from the UUID lookup map, so computers ended up in a phantom
state where they were destroyed, but still accessible.
This is not an issue on 1.20.4, because the legacy int lookup map was
removed.
Fixes#1760
BYTECODE WAS NOT SUPPOSED TO BE REWRITTEN
YEARS OF DEBUGGING REMAPPING FAILURES yet NO ACTUAL SOLUTION FOUND.
Wanted to use Mixins for anyway for a laugh? We had a tool for that: it
was called "FABRIC LOOM".
"Yes, please produce completely broken jars for no discernable reason"
Statements dreamed up by the utterly Deranged.
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
This removes our two mixins used on Forge:
- Breaking progress for cabled/wired modems.
- Running client commands from chat click events. We now suggest the
command on Forge instead.
Occasionally we get issues where the mixin annotation processor doesn't
write its tsrg file in time for the reobfJar/reobfJarJar task. I thought
we'd fixed that cb8e06af2aefd931b1f6743849a7afb2a25ea0bd, but sometimes
we still produce missing jars - I have a feeling this might be to do
with incremental compilation.
We can maybe re-evaluate this on 1.20.4, where we don't need to worry
about remapping any more.
Due to the asynchronous nature of main-thread tasks, it's possible for
them to be executed on peripherals which have been detached. This has
been known for a long time (#893 was opened back in 2021), but finding a
good solution here is tricky.
Most of the time the method will silently succeed, but if we try to
interact with an IComputerAccess (such as in inventory methods, as seen
in #1750), we throw a NotAttachedException exception and spam the logs!
This is an initial step towards fixing this - when calling a peripheral
method via peripheral.call/modem.callRemote, we now wrap any enqueued
main-thread tasks and silently skip them if the peripheral has been
detached since.
This means that peripheral methods may start to return nil when they
didn't before. I think this is *fine* (though not ideal for sure!) - we
return nil if the peripheral has been detached, so it's largely
equivalent to that.
This should never happen, but apparently it does!? We now log an error
(rather than crashing), and include the original BE (and associated
block), as the BE type isn't very useful.
See #1750. Technically this fixes it, but want to do some more poking
there first.
Here's a fun bug you can try at home:
- Create a new world
- Spawn in a pocket computer, turn it on, and place it in a chest.
- Reload the world - the pocket computer in the chest should now be
off.
- Spawn in a new pocket computer, and turn it on. The computer in chest
will also appear to be on!
This bug has been present since pocket computers were added (27th March,
2024).
When a pocket computer is added to a player's inventory, it is assigned
a unique *per-session* "instance id" , which is used to find the
associated computer. Note the "per-session" there - these ids will be
reused if you reload the world (or restart the server).
In the above bug, we see the following:
- The first pocket computer is assigned an instance id of 0.
- After reloading, the second pocket computer is assigned an instance
id of 0.
- If the first pocket computer was in our inventory, it'd be ticked and
assigned a new instance id. However, because it's in an inventory, it
keeps its old one.
- Both computers look up their client-side computer state and get the
same value, meaning the first pocket computer mirrors the second!
To fix this, we now ensure instance ids are entirely unique (not just
per-session). Rather than sequentially assigning an int, we now use a
random UUID (we probably could get away with a random long, but this
feels more idiomatic).
This has a couple of user-visible changes:
- /computercraft no longer lists instance ids outside of dumping an
individual computer.
- The @c[instance=...] selector uses UUIDs. We still use int instance
ids for the legacy selector, but that'll be removed in a later MC
version.
- Pocket computers now store a UUID rather than an int.
Related to this change (I made this change first, but then they got
kinda mixed up together), we now only create PocketComputerData when
receiving server data. This makes the code a little uglier in some
places (the data may now be null), but means we don't populate the
client-side pocket computer map with computers the server doesn't know
about.
- Remove "initial connections" flag, and just refresh connections +
peripherals on the first tick.
- Remove "peripheral attached" from NBT, and just read/write it from
the block state. This might cause issues with #1010, but that's
sufficiently old I hope it won't!
Our GatedPredicate hack was clever, but also fundamentally didn't work.
The predicate is called before extraction, so if extraction fails (for
instance, canTakeItemThroughFace returns false), then we still think an
item has been removed.
To fix that, we inline StorageUtil.move, specialising it for what we
need.
This feels a little overkill, but nice to standardise how this code
looks.
There's a bit of me which wonders if we should remove
IPeripheral.equals, and just use Object.equals, but I do also kinda like
the explicitness of the current interface? IDK.
This ensures the client decoder is in sync with the server. Well, mostly
- we don't handle the anti-jerk, but that should correct itself within a
few samples.
Fixes#1748
This adds support for computer selectors, in the style of entity
selectors. The long-term goal here is to replace our existing ad-hoc
selectors. However, to aid migration, we currently support both - the
previous one will most likely be removed in MC 1.21.
Computer selectors take the form @c[<key>=<value>,...]. Currently we
support filtering by id, instance id, label, family (as before) and
distance from the player (new!). The code also supports computers within
a bounding box, but there's no parsing support for that yet.
This commit also (finally) documents the /computercraft command. Well,
sort of - it's definitely not my best word, but I couldn't find better
words.
When rendering non-origin monitors, we would fetch the origin monitor,
read its client state, and then cache that on the current monitor to
avoid repeated lookups.
However, if the origin monitor is unloaded/removed on the client, and
then loaded agin, this cache will be not be invalidated, causing us to
render both the old and new monitor!
I think the correct thing to do here is cache the origin monitor. This
allows us to check when the origin monitor has been removed, and
invalidate the cache if needed.
However, I'm wary of any other edge cases here, so for now we do
something much simpler, and remove the cache entirely. This does mean
that monitors now need to perform extra block entity lookups, but the
performance cost doesn't appear to be too bad.
Fixes#1741
Historically, computers tracked whether any world-visible state
(on/off/blinking, label and redstone outputs) had changed with a single
"has changed" flag. While this is simple to use, this has the curious
side effect of that term.setCursorBlink() or os.setComputerLabel() would
cause a block update!
This isn't really a problem in practice - it just means slightly more
block updates. However, the redstone propagation sometimes causes the
computer to invalidate/recheck peripherals, which masks several other
(yet unfixed) bugs.
Minecraft sometimes keeps chunks in-memory, but not actively loaded. If
we schedule a block entity to be ticked and that chunk is is then
transitioned to this partially-loaded state, then the block entity is
never actually ticked.
This is most visible with monitors. When a monitor's contents changes,
if the monitor is not already marked as changed, we set it as changed
and schedule a tick (see ServerMonitor). However, if the tick is
dropped, we don't clear the changed flag, meaning subsequent changes
don't requeue the monitor to be ticked, and so the monitor is never
updated.
We fix this by maintaining a list of block entities whose tick was
dropped. If these block entities (or rather their owning chunk) is ever
re-loaded, then we reschedule them to be ticked.
An alternative approach here would be to add the scheduled tick directly
to the LevelChunk. However, getting hold of the LevelChunk for unloaded
blocks is quiet nasty, so I think best avoided.
Fixes#1146. Fixes#1560 - I believe the second one is a duplicate, and
I noticed too late :D.
Forge doesn't run client-side commands from sendUnsignedCommand, so we
still require a mixin there.
We do need to change the command name, as Fabric doesn't properly merge
the two command trees.
Disk drives have had a long-standing issue with mutating their contents
on the computer thread, potentially leading to all sorts of odd bugs.
We tried to fix this by moving setDiskLabel and the mounting code to run
on the main thread. Unfortunately, this means there is a slight delay to
mounts being attached, breaking disk startup.
This commit implements an alternative solution - we now do mounting on
the computer thread again. If the disk's stack is modified, we update it
in the peripheral-facing item, but not the actual inventory. The next
time the disk drive is ticked, we then sync the two items.
This does mean that there is a fraction of a tick where the two will be
out-of-sync. This isn't ideal - it would potentially be possible to
cycle through disk ids - but I don't really think that's avoidable
without significantly complicating the IMedia API.
Fixes#1649, fixes#1686.
- Add support for version overrides/exclusions in our dependency check.
Sometimes mod loaders use different versions to vanilla, and we need
some way to handle that.
- Rescan wired network connections on the tick after invalidation,
rather than when invalidated.
- Convert some constant lambdas to static method references. Lambdas
don't allocate if they don't capture variables, so this has the same
performance and is a little less ugly.
- Small code-style/formatting changes.
Historically we used Forge's SimpleChannel methods (and
PacketDistributor) to send the packets to the client. However, we don't
need to do that - it is sufficient to convert it to a vanilla packet,
and send the packet ourselves.
Given we need to do this on Fabric, it makes sense to do this on Forge
as well. This allows us to unify (and thus simplify) a lot of how packet
sending works.
At the same time, we also remove the handling of speaker audio during
decoding. We originally did this to avoid the additional copy of audio
data. However, this doesn't work on 1.20.4 (as packets aren't
encoded/decoded on singleplayer), so it makes sense to do this
Correctly(TM).
This also allows us to get rid of ClientNetworkContext.get(). We do
still need to service load this class (as Forge's networking isn't split
up in the same way Fabric's is), but we'll be able to drop that in
1.20.4.
Finally, we move the record playing code from ClientNetworkContext to
ClientPlatformHelper. This means the network context no longer needs to
be platform-specific!
After embarrassing, let's do some proper work.
Rather than passing the level and position each time we call
ComponentAccess.get(), we now pass them at construction time (in the
form of the BE). This makes the consuming code a little cleaner, and is
required for the NeoForge changes in 1.20.4.
Everything old is new again!
CC's network message implementation has gone through several iterations:
- Originally network messages were implemented with a single class,
which held an packet id/type and and opaque blobs of data (as
string/int/byte/NBT arrays), and a big switch statement to decode and
process this data.
- In 42d3901ee37892e259de26ebb57cf59ce284416e, we split the messages
into different classes all inheriting from NetworkMessage - this bit
we've stuck with ever since.
Each packet had a `getId(): int` method, which returned the
discriminator for this packet.
- However, getId() was only used when registering the packet, not when
sending, and so in ce0685c31f7315d15d3250c6c8605171b33aa99f we
removed it, just passing in a constant integer at registration
instead.
- In 53abe5e56eec6840890770b6ec36a5d009357da7, we made some relatively
minor changes to make the code more multi-loader/split-source
friendly. However, this meant when we finally came to add Fabric
support (8152f19b6efd71b66c3821ad94aacaddb7d26298), we had to
re-implement a lot of Forge's network code.
In 1.20.4, Forge moves to a system much closer to Fabric's (and indeed,
Minecraft's own CustomPacketPayload), and so it makes sense to adapt to
that now. As such, we:
- Add a new MessageType interface. This is implemented by the
loader-specific modules, and holds whatever information is needed to
register the packet (e.g. discriminator, reader function).
- Each NetworkMessage now has a type(): MessageType<?> function. This
is used by the Fabric networking code (and for NeoForge's on 1.20.4)
instead of a class lookup.
- NetworkMessages now creates/stores these MessageType<T>s (much like
we'd do for registries), and provides getters for the
clientbound/serverbound messages. Mod initialisers then call these
getters to register packets.
- For Forge, this is relatively unchanged. For Fabric, we now
`FabricPacket`s.
While ComputerFamily is still useful, there's definitely some places
where it adds an extra layer of indirection. This commit attempts to
clean up some places where we no longer need it.
- Remove ComputerFamily from AbstractComputerBlock. The only place this
was needed is in TurtleBlock, and that can be replaced with normal
Minecraft explosion resistence!
- Pass in the fuel limit to the turtle block entity, rather than
deriving it from current family.
- The turtle BERs now derive their model from the turtle's item, rather
than the turtle's family.
- When creating upgrade/overlay recipes, use the item's name, rather
than {pocket,turtle}_family. This means we can drop getFamily() from
IComputerItem (it is still needed on to handle the UI).
- We replace IComputerItem.withFamily with a method to change to a
different item of the same type. ComputerUpgradeRecipe no longer
takes a family, and instead just uses the result's item.
- Computer blocks now use the normal Block.asItem() to find their
corresponding item, rather than looking it up via family.
The above means we can remove all the family-based XyzItem.create(...)
methods, which have always felt a little ugly.
We still need ComputerFamily for a couple of things:
- Permission checks for command computers.
- Checks for mouse/colour support in ServerComputer.
- UI textures.
When adding/removing observers, we locked on the observer, then
acquired the global lock. When a metric is observed, then we acquire the
global lock and then the observer lock.
If these happen at the same time, we can easily end up with a deadlock.
We simply avoid holding the observer lock for the entire add/remove
process (instead only locking when actually needed).
Closes#1639
This commit adds abstract classes to describe the interface for our
mod-loader-specific generic peripherals (inventories, fluid storage,
item storage).
This offers several advantages:
- Javadoc to illuaminate conversion no longer needs the Forge project
(just core and common).
- Ensures we have a consistent interface between Forge and Fabric.
Note, this does /not/ implement fluid or energy storage for Fabric. We
probably could do fluid without issue, but not something worth doing
right now.
Historically CC has supported two modes when working with file handles
(and HTTP requests):
- Text mode, which reads/write using UTF-8.
- Binary mode, which reads/writes the raw bytes.
However, this can be confusing at times. CC/Lua doesn't actually support
unicode, so any characters beyond the 0.255 range were replaced with
'?'. This meant that most of the time you were better off just using
binary mode.
This commit unifies text and binary mode - we now /always/ read the raw
bytes of the file, rather than converting to/from UTF-8. Binary mode now
only specifies whether handle.read() returns a number (and .write(123)
writes a byte rather than coercing to a string).
- Refactor the entire handle hierarchy. We now have an AbstractMount
base class, which has the concrete implementation of all methods. The
public-facing classes then re-export these methods by annotating
them with @LuaFunction.
These implementations are based on the
Binary{Readable,Writable}Handle classes. The Encoded{..}Handle
versions are now entirely removed.
- As we no longer need to use BufferedReader/BufferedWriter, we can
remove quite a lot of logic in Filesystem to handle wrapping
closeable objects.
- Add a new WritableMount.openFile method, which generalises
openForWrite/openForAppend to accept OpenOptions. This allows us to
support update mode (r+, w+) in fs.open.
- fs.open now uses the new handle types, and supports update (r+, w+)
mode.
- http.request now uses the new readable handle type. We no longer
encode the request body to UTF-8, nor decode the response from UTF-8.
- Websockets now return text frame's contents directly, rather than
converting it from UTF-8. Sending text frames now attempts to treat
the passed string as UTF-8, rather than treating it as latin1.
In practice, we're never going to change this to true by default. The
old Tekkit Legends pack enabled this[^1], and that caused a lot of
problems, though admittedly back in 2016 so things might be better now.
If people do want this functionality, it should be fairly easy to
replicate with a datapack, adding a file to rom/autorun.
[^1]: See https://www.computercraft.info/forums2/index.php?/topic/27663-
Hate that I remember this, why is this still in my brain?
- Move most error message constants to a new MountHelpers class.
- Be a little more consistent in when we throw "No such file" vs "Not a
file/directory" messages.
The two implementations aren't entirely compatible - the implementation
returned by .of will throw an NPE on .contains(null), whereas the
Collections implementations just return false. However, we try to avoid
passing null to collections methods, so this should be safe.
There's no strong reason to do this, but it helps make the code a little
more consistent
As of 1.20, sign messages are immutable - we need to do
text = text.setMesssage(...) instead. Also do a tiny bit of cleanup to
this function while we're here.
Probably not the best use of my lunch break :D:.
Fixes#1611.
Allows registering arbitrary block lookup functions instead of a
platform-specific capability. This is roughly what Fabric did before,
but generalised to also take an invalidation callback.
This callback is a little nasty - it needs to be a NonNullableConsumer
on Forge, but that class isn't available on Fabric. For now, we make the
lookup function (and thus the generic peripheral provider) generic on
some <T extends Runnable> type, then specialise that on the Forge side.
Hopefully we can clean this up when NeoForge reworks capabilities.
Wow, some of this is /old/. All the Maps.newHashMap stuff dates back to
Java 6, so must originally be CCTweaks code?!
We're unlikely to drop our Guava dependency (we use too much other
stuff), but we should make the most of the stdlib where possible.
Previously we had the invariant that if we had a server monitor, we also
had a terminal. When a monitor shrank into a place, we deleted the
monitor, and then recreated it when a peripheral was requested.
As of ab785a090698e6972caac95691393428c6f8370b this has changed
slightly, and we now just delete the terminal (keeping the ServerMonitor
around). However, we didn't adjust the peripheral code accordingly,
meaning we didn't recreate the /terminal/ when a peripheral was
requested.
The fix for this is very simple - most of the rest of this commit is
some additional code for ensuring monitor invariants hold, so we can
write tests with a little more confidence.
I'm not 100% sold on this approach. It's tricky having a double layer of
nullable state (ServerMonitor, and then the terminal). However, I think
this is reasonable - the ServerMonitor is a reference to the multiblock,
and the Terminal is part of the multiblock's state.
Even after all the refactors, monitor code is still nastier than I'd
like :/.
Fixes#1608
We can't use FriendlyByte.readCollection to read to a
pre-allocated/array-backed NonNullList, as that doesn't implement
List.add. Instead, we just need to do a normal loop.
We add a couple of tests to round-trip our recipe specs. Unfortunately
we can't test the recipes themselves as our own registries aren't set
up, so this'll have to do for now.
Oh, this was a really nasty bug to reproduce. I'm not sure why - it's
very simple - I guess I've only just seen screenshots of it, and never
sat down to try myself. Reminder to actually report your bugs folks!
In this case:
1. Place down three down three monitors and then a computer.
2. Display something on the monitor (monitor left paint a) is my go-to.
3. Break the middle monitor.
We'd expect the left most monitor to be cleared, however it actually
preserves the monitor contents, resizing (and skewing it) to fit on its
new size!
This is because we clear the server monitor, but never sync that over to
the client, so the client monitor retains the old contents. To fix that,
instead of nulling out the server monitor, we null out the underlying
Terminal. This causes the change to be synced, fixing the bug.
