SuperMartijn642 / Tesseract

9 stars 9 forks source link

Fix LazyOptional Capability invalidation bug. #84

Closed hyfloac closed 11 months ago

hyfloac commented 11 months ago

Problem

On Forge 1.19.2, under certain conditions the Tesseract does not correctly bind to a duct/pipe. This can be semi-resolved by destroying the entire previous duct network along with all devices, destroying the Tesseract, re-placing the Tesseract, connecting it to a Channel, then rebuilding the duct network, though this is still finicky.

Cause

Within TesseractBlockEntity there are 2 places where Capabilities are retrieved, getCapability which returns the Capabilities of the Tesseract, and getSurroundingCapabilities which returns the Capabilities of the neighboring blocks. In both cases these Capabilities are wrapped in LazyOptional's, and are queried from a Map using computeIfAbsent. The problem is that computeIfAbsent is only designed to handle the value stored in the Map being null, but it is not capable of handling a LazyOptional which has been invalidated.

There seems to a point at which the duct networks get invalidated, the capabilities stored within the Tesseract effectively become null, but the getCapability and getSurroundingCapabilties functions are not cognizant of that.

Resolution

In order to properly handle LazyOptional's a wrapper function was added.

/**
 *   A replacement wrapper for {@link Map#computeIfAbsent(Object, Function)}
 * that can handle a {@link LazyOptional} being invalidated.
 *
 * @param map A mapping between a generic key and a value wrapped in a
 *          LazyOptional.
 * @param key The key to test for.
 * @param mappingFunction The mapping function to execute if the value
 *           is missing or invalidated. This function should probably
 *           not return null, instead it should probably return
 *           {@link LazyOptional#empty}.
 * @param <K> The generic key type.
 * @return The value associated with the key (either pre-existing, or
 *      newly created if the value was previously missing or
 *      invalidated) wrapped in a LazyOptional. This can be null, if
 *      the mapping function returns a null, though it shouldn't.
 */
private static <K> LazyOptional<?> computeIfLazyAbsent(Map<K, LazyOptional<?>> map, K key, Function<? super K, ? extends LazyOptional<?>> mappingFunction){
    // If the value is fully missing, defer to the original functionality of Map.
    if(!map.containsKey(key)){
        return map.computeIfAbsent(key, mappingFunction);
    }

    LazyOptional<?> value = map.get(key);

    // If the value is null, defer to the original functionality of Map.
    if(value == null){
        return map.computeIfAbsent(key, mappingFunction);
    }

    // If the value is present, there is no need to perform the mapping.
    if(value.isPresent()){
        return value;
    }

    // Create the new value.
    value = mappingFunction.apply(key);

    // If the value is not null (which should always be true), store it into the map.
    if(value != null){
        map.put(key, value);
    }

    return value;
}

Testing

This has been tested within the following environments:

In all tests the Tesseract was able to successfully connect with the ThermalDynamics fluxduct & Mekanism Logistics Transporter networks without problem. The network could change, or be destroyed and rebuilt, and the Tesseract would continue to function. This had a positive effect on both the actual transfer of items and power, as well as the visual connection of ducts and transporters.

Other Minecraft Versions

This change was specifically made to the MC version 1.19.2, but should be easily applicable to other Minecraft versions. Here is a patch file that can be directly applied to the TesseractBlockEntity file in the 1.19 branch, but should be applyable to other similar versions, and I have been able to successfully run git apply to the 1.19.4 branch.

Other

Some other minor things I noticed while debugging this:

SuperMartijn642 commented 11 months ago

Wow, that is quite the oversight. No clue how I missed that. Hopefully that fixes some of the jankiness players tend to experience.

Your solution looks good to me. Thank you for looking into it and the excellent report and the testing! 😊


The gradle-wrapper.properties file points to gradle being distributed from downloads.gradle-dn.com which seems to be compromised, downloads.gradle.org is working though.

Ah I still have the files cached somewhere, so I am just slowly replacing all those old links as the caches get invalidated 😅

The CoFH team has a maven server at https://maven.covers1624.net/, adding their dependencies from their server rather than Curse will get you dependency jars with sources attached.

I just need to load them for the recipe data gen pass as there a Thermal series compat recipe, so not having sources is fine. No need for them to even be implementation really as runtimeOnly is also fine.