#1.20.2

1 messages · Page 4 of 1

mental carbon
#

It does not set the permissions properly

#

So i got to redo this AGAIN

#

Actually nvm

#

Wrong org

#

ARG

#

Done

dull bison
#

neoify is pushed

fallow sundial
mental carbon
mental carbon
mental carbon
#

Okey created a patch for Srg2Source

#

Had to rip out ProGuard

#

But got an updated compile target

sly anvil
#

Possible this question should wait until 1.20.2 is out, but what's the plan for ChunkWatchEvent.Watch with the chunk networking changes?

sturdy phoenix
#

depends on how long this moment is

sly anvil
#

Ah, thanks! Had searched for the class name, not the one without spaces :)

mental carbon
#

We fixed all the networking problems, but in reality we have more of a problem with the complete reimplementation of ChunkMap which is entirely re-written, and now uses section pos.

sly anvil
#

What's the difference between sections and chunks again? Are sections the 16^3 region?

mental carbon
#

Yep

#

It is a relatively easy fix

#

But we still have not found any nice place to wire up the events

sly anvil
#

Yeah. I mostly ask because the Watch event is used for sending additional data alongside the chunk, and for tracking watching changes, so somewhat wondering if it makes sense to split the event instead.

mental carbon
#

Personally I don't know.....

#

The chunkmap is currentls still above my capabilities and time availability to understand

sly anvil
#

Oh yeah, definitely not the main thing to focus on right now, mostly being nosey. Thank you!

tacit vale
mental carbon
#

No...... Not at the moment......

kindred fractal
#

Nothing was done for configuration afaik

dull bison
#

orion did you manage to genPatches yet? I added the access changes as AT entries to the forge AT (they still need to be placed in the right place because I just put them at the end of the file)

mental carbon
#

Not yet

fallow sundial
#

was networking fixed

mental carbon
#

Yes

#

At least it is functional

#

But the proposed protocol change was not implemented

tacit vale
#

but you will right?

mental carbon
#

When I get the time

stray scroll
#

i just got the mental image of that one Star Wars meme with Padme asking Anakin thinkies

kindred hatchBOT
#

[Jump to referenced message.](#minecraft-updates message)

gigaherz referenced ·
inland mesa
#

ugh ... #minecraft-updates message

#

can someone make camelot say what channel the message is from? :P

fallow sundial
inland mesa
#

too late it already deleted my message :P

#

but the ugh is more that it doesn't embed the embeds so it doesn't show the context :P

fallow sundial
#

ah yes embed in an embed

#

that would be intersting

inland mesa
#

well if it can't then I would hope for like, a hint to what the target is :p

#

anyhow pre4 is out

#

have fun porters

fallow sundial
#

who's got a psychic api /s

inland mesa
#

do the bell and all

fallow sundial
#

<#neoforge message>

Update to java 17, and latest JDT
suspicious @mental carbon did you make sure that s2s is run with j17 using toolchains?

mental carbon
#

Yes

dull bison
#

orion I don't think these are correct

image.png
keen hearth
#

checkAt's is yet to be run

dull bison
#

I did and I think it checks with the wrong mappings because it flags the correct ones as invalid

keen hearth
#

Interesting

dull bison
#

it also says it is missing group entries but when I check which field that is with the bot it says it doesn't exist (using 1.20.2-pre1 for the bot mappings)

keen hearth
#

iirc the groups are re-generated, when running checkAts

#

does the new field or an old field not exist?

dull bison
#

?

mental carbon
dull bison
#

why is there a missmatch between patches and bot then

#

!moj BuiltInPackSource.fixedResources 1.20.2-pre1

mild topazBOT
#

Unexpected error occurred dispatching command. Please report this to your bot admin.

mental carbon
fallow sundial
#

the bot doesn't know about snapshots

#

only releases

mental carbon
#

But I know that those worked without issue on my end

#

And the ones that the bot produced did not

#

So I went with the ones in the patches

dull bison
mental carbon
#

I knows about the snapshot

#

But i have no idea if it knows the srg names or ids for it

#

Actually NeoForm ids

dull bison
#

so it is a missmatch between mcp srg and neoform srg?

mental carbon
#

Maybe

#

I don't know

fallow sundial
#

plus it looks it up from the forge maven, not the neo one. I'll try to update it but also neoform doesn't have non-timestamped versions so it's going to be a bit more painful and seems like useless work if we're soon™️ getting rid of srg

frail oriole
#

yeah

#

that's the k9 bot right?

#

the one we're getting rid of?

#

as well as the removal of srg

#

this seems like multiple layers of pointless

fallow sundial
frail oriole
#

fair

keen hearth
#

Hey, thats a cool site!

#

bookmarks

frail oriole
#

hmmm

#

which one is the mojang standard mappings tho?

keen hearth
#

Okay, all of client package patch auditedcleaned.

frail oriole
#

question: why are there so many mapping targets

dull bison
#

I'm doing checkAT's

fallow sundial
frail oriole
#

mojmaps should be listed there IMO

#

esp since that's gonna be our target soon

fallow sundial
#

@thorn robin take notes

#

also, for worse or for better, linkie web allows you to view vanilla sources lol

image.png
fallow sundial
#

and each have their own intermediary

keen hearth
#

Uhh, it probably shouldn't..

#

That's a C&D waiting to happen..

dull bison
fallow sundial
#

because srg doesn't have public snapshot versions

dull bison
#

neoform

dull bison
#

I think we are done with pre1

inland mesa
#

nice nice

#

pre2 3 4 time

fallow sundial
#

4 ate 2 3

mental carbon
#

Not completly

#

But yeah I will start the process of pre4 in about 20 minutes

inland mesa
#

3 and 4 seemed rather minor (specially 4)

#

based on changelog

rich void
#

maybe a lot of coding changes?

#

it seemed they were working on sum stuff

dull bison
#

I believe we skip 2 and 3

dull bison
#

orion don't forget to push

mental carbon
#

Yeah I am not done cleaning stuff

#

@dull bison This is not a good solution:

# 
   public static final Codec<Ingredient> CODEC = net.minecraftforge.common.util.ForgeExtraCodecs.withAlternative(
         ExtraCodecs.lazyInitializedCodec(() -> net.minecraftforge.common.crafting.CompoundIngredient.DIRECT_CODEC.flatComapMap(java.util.function.Function.identity(), i -> i instanceof net.minecraftforge.common.crafting.CompoundIngredient c ? DataResult.success(c) : DataResult.error(() -> "Not a compound ingredient"))),
         net.minecraftforge.common.util.ForgeExtraCodecs.withAlternative(
               REGISTRY_CODEC,
               VANILLA_CODEC
         )
   );
#

We can not special case the compound like that

#

We need a way to deal with this

#

Other then just special casing it I am sorry

mossy bobcat
#

That's just for the direct compound ingredient based on a list, right? The third one of these: #1136320550168436798 message

mental carbon
#

Yep but the simple answer is: That is not a valid option

mossy bobcat
#

Only alternative is to just not allow that syntax

mental carbon
#

Period

#

Given that the compound type is not the vanilla type, their is no way you can leave out the type in the first place

#

As such I see no option other then to disallow it

mossy bobcat
#

I mean, the current special casing of it works, but I guess the worry is that it's confusing as it's not a vanilla thing?

mental carbon
#

Correct, and we do not allow any other none vanilla ingredient, or other subsystem for that matter, to do this

#

To override the "default" tag just because it is not an array or an object

mossy bobcat
#

I mean yeah but, and I can't believe I'm the one saying this, doesn't neoforge normally special case itself relative to other mods all over the place? Like, I'm all for not giving neoforge special treatment in this regard but I feel like you'd have to start doing that everywhere to be consistent

#

But yeah, solution is to just not allow that syntax

dull bison
muted hemlock
#

err

#

is the compound ingredient vanilla syntax or not vanilla syntax

muted hemlock
dull bison
#

the type CompoundIngredient is forge but the list syntax is vanilla but without that special casing can only do item and tag values

muted hemlock
#

oh thonk

#

hmm

mossy bobcat
mossy bobcat
#

As otherwise that's even more confusing

quiet talon
#

Actually the list syntax for vanilla is only items

mossy bobcat
#

Wait wut? Mojank I guess

muted hemlock
#

1.20.3: "ingredients now use holdersets"

quiet talon
# 
               return fromValues(StreamSupport.stream(jsonarray.spliterator(), false).map((p_289756_) -> {
                  return valueFromJson(GsonHelper.convertToJsonObject(p_289756_, "item"));
               }));
mental carbon
#

Given that vanilla has the format claimed

muted hemlock
#

but yeah I concur that all non-vanilla ingredient formats should be a json object with an explicit type field

quiet talon
#

oh nevermind that's just a horrible method name

mental carbon
#

That is a big no vote from me on the cusotm compound handling

dull bison
mental carbon
#

if you need compound based handling then specify it

quiet talon
#

it treats the array entries as Objects, the usage of "item" there is only for error logging despite it being totally irrelevant

mossy bobcat
#

So, to clarify: what does the list format allow with the special casing that it doesn't in vanilla?

dull bison
#

if list -> compound

quiet talon
#

I believe our extensions allow the list to be any ingredient

#

Otherwise vanilla only calls valueFromJson for the array elements

#

which cannot produce a full Ingredient

mossy bobcat
#

Because best I can tell this special casing makes perfect sense? You're basically abstracting the vanilla ingredient behavior, and when deciding how the vanilla list fits into that you say it's a compound, which fits into what vanilla does

dull bison
mossy bobcat
#

But to clarify further: what's the actual codec used for members of that list in vanilla?

muted hemlock
#

well.. does the vanilla list ingredient use a list codec of the ingredient codec?

dull bison
#

the list members in vanilla are Ingredient.Value

mossy bobcat
#

Hmm. Then I wouldn't special case it

muted hemlock
dull bison
#

and the special casing changes that to be Ingredieng

dull bison
muted hemlock
#

aw

mossy bobcat
#

If the vanilla intended behavior was "union of ingredients" they'd use the root ingredient codec directly. This would allow, like, nesting of lists which seems weird

quiet talon
# 
private static Codec<Ingredient> codec(boolean p_301074_) {
      Codec<Ingredient.Value[]> codec = Codec.list(Ingredient.Value.CODEC)
         .comapFlatMap(
            p_300810_ -> !p_301074_ && p_300810_.size() < 1
                  ? DataResult.error(() -> "Item array cannot be empty, at least one item must be defined")
                  : DataResult.success(p_300810_.toArray(new Ingredient.Value[0])),
            List::of
         );
      return ExtraCodecs.either(codec, Ingredient.Value.CODEC)
         .flatComapMap(
            p_300805_ -> p_300805_.map(Ingredient::new, p_300806_ -> new Ingredient(new Ingredient.Value[]{p_300806_})),
            p_300808_ -> {
               if (p_300808_.values.length == 1) {
                  return DataResult.success(Either.right(p_300808_.values[0]));
               } else {
                  return p_300808_.values.length == 0 && !p_301074_
                     ? DataResult.error(() -> "Item array cannot be empty, at least one item must be defined")
                     : DataResult.success(Either.left(p_300808_.values));
               }
            }
         );
   }

it does this™️

dull bison
#

that is why the special casing is needed

quiet talon
#

Vanilla doesn't have Ingredient subclasses, only Value

#

so vanilla wouldn't use a list of the root, because there is only a single ingredient object, which is one or more Values

mossy bobcat
#

Hmm. Yeah, that's a weird case then

quiet talon
#

But since we have custom Ingredients, we should expand the array syntax to support full ingredient objects in the array

mossy bobcat
#

Here is what I would say: in vanilla, the list and the single item codec that are combined with an either are the same

#

We should have the same behavior

#

So, if vanilla is "value or list of values", then we should have "possibly custom value or list of possibly custom values"

quiet talon
#

Wouldn't that make everything a CompoundIngredient?

#

that sounds like a nightmare

mossy bobcat
#

No, not necessarily

#

I'm talking codec general structure, not what it parses to

mental carbon
#

Lets just make the CI need a type

mossy bobcat
#

Basically, I'm saying keep the special casing

mental carbon
#

That takes all guessing out of the room

dull bison
mental carbon
#

For every other system we have that you need to supply a type

#

To get the custom logic

mossy bobcat
#

Because whatever I'm allowed to put into the root thing, I should be allowed to put into the list

mossy bobcat
#

So, your special casing of this is correct

#

Basically, if I can do```json
"ingredient": {
"type": "whatever",
...
}

Then I should be able to put that same structure inside of a list and have it Just Work™️. And I believe that requires this special casing to use compound ingredients
mental carbon
#

Because any other forge system uses the vanilla logic for the "no type" specified scenario

mossy bobcat
#

But frankly, anything else is confusing

mental carbon
#

Even if it is a list

quiet talon
#

Yeah, it will be very awkward if

"input": [
  {
    "item": "minecraft:stick",
  },
  {
    "tag": "#minecraft:cobblestone"
  }
]

is legal but

"input": [
  {
    "type": "forge:partial_nbt",
    "item": "minecraft:stick",
    "nbt": "{\"blah\": 10}"
  },
  {
    "tag": "#minecraft:cobblestone"
  }
]

is not

mental carbon
quiet talon
#

since it is legal now

mossy bobcat
#

Because that's fucking confusing, that's why

mental carbon
#

Yeah, and I am arguing that it should not be legal now

dull bison
#

this is what we did in 1.20.1

    public static Ingredient getIngredient(JsonElement json, boolean allowEmpty)
    {
        if (json == null || json.isJsonNull())
            throw new JsonSyntaxException("Json cannot be null");

        if (json.isJsonArray())
        {
            List<Ingredient> ingredients = Lists.newArrayList();
            List<Ingredient> vanilla = Lists.newArrayList();
            json.getAsJsonArray().forEach((ele) ->
            {
                Ingredient ing = CraftingHelper.getIngredient(ele, allowEmpty);

                if (ing.getClass() == Ingredient.class) //Vanilla, Due to how we read it splits each itemstack, so we pull out to re-merge later
                    vanilla.add(ing);
                else
                    ingredients.add(ing);
            });

            if (!vanilla.isEmpty())
                ingredients.add(Ingredient.merge(vanilla));

            if (ingredients.size() == 0)
            {
                if (allowEmpty)
                {
                    return Ingredient.EMPTY;
                }
                throw new JsonSyntaxException("Item array cannot be empty, at least one item must be defined");
            }

            if (ingredients.size() == 1)
                return ingredients.get(0);

            return new CompoundIngredient(ingredients);
        }

        if (!json.isJsonObject())
            throw new JsonSyntaxException("Expcted ingredient to be a object or array of objects");

        JsonObject obj = (JsonObject)json;

        String type = GsonHelper.getAsString(obj, "type", "minecraft:item");
        if (type.isEmpty())
            throw new JsonSyntaxException("Ingredient type can not be an empty string");

        IIngredientSerializer<?> serializer = ingredients.get(new ResourceLocation(type));
        if (serializer == null)
            throw new JsonSyntaxException("Unknown ingredient type: " + type);

        return serializer.parse(obj);
    }
mental carbon
#

Since I find that confusing as all hell

#

Because now all of a sudden

#

Were that original syntax was previously a normal ItemPredicate

#

It is now something completely different in code

quiet talon
#

The expansion for Ingredients is to be able to deserialize Ingredient instead of Value - that should be consistent across all places that deserialize Value, including this array syntax.

mossy bobcat
mental carbon
#

That is were your understanding is wrong

#

It allows a single ingredient value, or a list of ingredient values!

#

It does not allow a single ingredient, or a list of ingredients!

#

It never had

quiet talon
#

in vanilla those are the same thing, orion

mental carbon
#

We only made it that

#

No they are not shadows

#

Especially now

#

Where they explicitly have custom intermediary logic to deal with tags and other options

mossy bobcat
#

Sure. But we're switching out the root ingredient value codec for a root ingredient codec based on dispatch, eithered with the value codec

mental carbon
#

We are switching out the codec for the ENTIRE ingredient

#

Not the values

#

The value parsing is identical if it detects a vanilla ingredient (so missing type, or it being an array)

mossy bobcat
#

Okay, I'm just saying that allowing this:```json
"ingredient": <whatever>

But only allowing this for certain whatever but not others:```json
"ingredient": [ <whatever> ]

Is just confusing, full stop

mental carbon
#

Why?

mossy bobcat
#

Because that's confusing? What makes the one special that it only works on the root context?

dull bison
mental carbon
#

The array always represents a vanilla ingredient with multiple values

quiet talon
#

that would be stupid ^

mental carbon
mental carbon
quiet talon
#

Because the array syntax exists

mental carbon
#

Note that the types that are parsed in the two cases are significantly different

quiet talon
#

and since it exists, it must be expanded in accordance with the non-array syntax

mossy bobcat
# mental carbon The array always represents a vanilla ingredient with multiple values

Okay. Let me explain. From a user perspective, who hasn't seen the ingredient class and just sees the vanilla ingredient types and forge's added ones, the difference between a "forge added ingredient type" and a "vanilla ingredient value type" at the root level is... Non-existent, basically. From what they can see there's a vanilla way of specifying an ingredient, and a forge extension. Now, they're used to being able to take that vanilla way of specifying an ingredient, best it in a list, and have it Just Work. If this suddenly doesn't work with forge's special things (which look equivalent from what they can see! Remember, they're both JSON Objects here, it's not like ones just a string or something), that is very confusing

quiet talon
#

Because if you don't do that, you are changing the definition of an ingredient from "one or more Values" to "Maybe one Ingredient, or more than one Value", when it should be changed to "one or more Ingredients"

mental carbon
#

It has always been there

#

In any other subsystem

#

If you need something other then vanilla behaviour

#

You need to specify the type

quiet talon
#

The type field expands Value to Ingredient, it must do this unequivocally, not partially

mental carbon
#

That has always been the common context of every custom forge data driven sytem

quiet talon
#

failing to do this in the list syntax becomes a partial implementation of our own spec

mental carbon
#

It never has shadows

#

It always specified the type of ingredient

mossy bobcat
# mental carbon In any other subsystem

Yes. But my point is that in other such systems, the thing represented by "thing without a type field" and "thing with a type field" aren't fundamentally different types of stuff

mental carbon
#

Never its values

quiet talon
#

it expands the parser from parsing Value to parsing Ingredient

mental carbon
mental carbon
quiet talon
#

it does this at the base level and at the array level
but whatever, you're being stubborn and I'm tired of arguing, You have three people telling you not to change it, so do not change it unless you pull a maintainer vote and get approvals.

mental carbon
#

It made the parser of the compound ingredient mimick vanilla behaviour

#

But it never expanded the value parsing it self!

mossy bobcat
muted hemlock
#

I would like to point out that, the way we expand holdersets, you can have a list of ids (a vanilla format), but you can't have a list of holdersets without using an explicitly-typed holderset

#

this sounds a lot like the ingredient problem

mental carbon
mossy bobcat
#

I'm trying to look at this from the "writing a datapack" perspective, ignoring what the code looks like

mental carbon
#

Which is what I am pointing out

#

Even for biome logic

mossy bobcat
mental carbon
#

And registry logic this all holds

mental carbon
muted hemlock
mental carbon
#

The user has to explicitly enable custom forge logic

#

Always

mossy bobcat
muted hemlock
#

I might have decided that thinkies

mossy bobcat
mental carbon
mental carbon
#

If we switch to the behaviour as request

#

Then any array will ALWAYS be a compound ingredient

#

Regardles off its contents

mossy bobcat
#

Okay, alternative idea here that solves this. Parse in a series of eithers, in this order:

  • single value
  • list of values
  • single ingredient
  • list of ingredients
mental carbon
#

And that is an implicit change that is not choosen by the user

mossy bobcat
#

That way arrays of just vanilla stuff are still parsed the vanilla way

muted hemlock
mental carbon
muted hemlock
#

it sounds like more work

mental carbon
#

IMHO: We should not, because it confuses work

mossy bobcat
muted hemlock
#

it would've been more work to expand the vanilla list holderset

mental carbon
dull bison
#

I just made the codec so it works with the recipes in the testmods

mental carbon
#

But my point is, I very much want consistency across forge features

#

And this is the thing that stands out in this case

mossy bobcat
#

If it's changed on the ingredient stuff it should be changed for holder sets too, assuming they're really equivalent, I agree

mental carbon
#

If you can come up with a functional argument why the array should be implicitly changed, without the consent of the user/datapack maker, then I am all for it

#

But not without

mossy bobcat
muted hemlock
#

the main reason the vanilla list holderset wasn't expanded was because I just made the eithering go like
"if it's an object, read it using the forge stuff, otherwise use the vanilla parser"

dull bison
mental carbon
muted hemlock
#

and also like... vanilla's list holdersets is not a list of holdersets

#

it doesn't even read tag ids, it can only be a list of single ids

dull bison
mental carbon
muted hemlock
#

right, I'm just trying to point out something analogous

mental carbon
#

If we send the ops from the recipe through

muted hemlock
dull bison
mental carbon
muted hemlock
#

oh, neat

mental carbon
#

We are basically one step away from passing in the entire registry ops with support for holder sets into the ingredient parser

muted hemlock
#

well... no, because ingredients can be nbt-sensitive

mental carbon
#

But we did not

muted hemlock
#

err I guess you could have holdersets as a type of ingredient

mental carbon
#

Because well they might be unforseen concequences

dull bison
mental carbon
#

No it is just pure JsonElement stuff still

#

There is no ops and element data currently available

dull bison
mental carbon
#

That is what is invoked from Recipe

mossy bobcat
#

I'm just looking at this from the perspective of "i haven't seen the class, why isn't my stuff working" and this seems extremely confusing if it doesn't allow custom stuff in lists. Like, codecs and JSON structure should not reflect code exclusively; they should make sense to someone who hasn't seen the code! There shouldn't be "gotchas" you have to watch out for introduced by forge changes, which is what this seems to do

mental carbon
#

Actually nevermind

dull bison
mental carbon
mossy bobcat
#

But if the worry is "don't want to parse vanilla stuff through forge path"... Just put the vanilla codec first in the either and it'll all go through that route first and parse any lists with only normal old ingredient values in it

muted hemlock
#

errr

#

is list of objects the vanilla format thonk

dull bison
muted hemlock
#

oh wow that does make it annoying

mossy bobcat
#

Yep, that's the issue here

#

Vanilla ingredient values are objects. Forge's fancy stuff are objects

muted hemlock
#

then yeah I think forge should allow forge ingredients in the vanilla list of objects

#

(if feasible)

mossy bobcat
#

You want to know something really silly?```json
{
"type": "custom:whatever",
"item": "minecraft:iron_ingot"
}

...this'll parse with the vanilla codec if the vanilla codec gets to it
muted hemlock
#

but as an iron ingot, not a whatever thinkies

mossy bobcat
#

Yep

muted hemlock
#

okay yeah we should do something about that

mossy bobcat
#

So this is doable but you have to do some extra thinking about how to not have Issues™️

dull bison
mossy bobcat
dull bison
#

the registry codec comes before the vanilla one

mossy bobcat
#

Which means that that same thing, used as an ingredient in forge and vanilla, would give different results

dull bison
#

it only would if there is no custom:whatever in the registry

mossy bobcat
#

In other words, custom ingredient types can break vanilla compat rather easily if anyone happens to register one using item or tag as a field

#

I honestly dislike this approach to custom ingredients in general for this reason

#

But it's not like there's many other good options

mental carbon
#

I mean

mossy bobcat
mental carbon
#

If this is really such a sticking point

#

We could leave it schurli

#

But IMHO we should not expand it

#

This is really not a good solution

muted hemlock
#

or have a steering council vote harold

mental carbon
#

And this format should never have been accepted and as such adapted in the first place

mossy bobcat
#

How feasible is it to have custom ingredient value types instead of custom ingredient types?

mental carbon
#

Due to the way the vanilla ingredient works

mossy bobcat
#

Oh screm there's mojank, right

dull bison
#

we can remove that special casing in 1.21

mossy bobcat
#

Personally I'd say "yeet custom ingredients entirely and rework it from the ground up"

#

Because personally we should follow vanilla, and delegate based on a field name (item, tag) not based on a type field value, to avoid possible overlaps

#

...and to avoid this #1136320550168436798 message

dull bison
# mossy bobcat How feasible is it to have custom ingredient value types instead of custom ingre...

may I present the Ingredient.Value interface

   public interface Value {
      Codec<Ingredient.Value> CODEC = ExtraCodecs.xor(Ingredient.ItemValue.CODEC, Ingredient.TagValue.CODEC)
         .xmap(p_300956_ -> p_300956_.map(p_300932_ -> p_300932_, p_301313_ -> p_301313_), p_301304_ -> {
            if (p_301304_ instanceof Ingredient.TagValue ingredient$tagvalue) {
               return Either.right(ingredient$tagvalue);
            } else if (p_301304_ instanceof Ingredient.ItemValue ingredient$itemvalue) {
               return Either.left(ingredient$itemvalue);
            } else {
               throw new UnsupportedOperationException("This is neither an item value nor a tag value.");
            }
         });

      Collection<ItemStack> getItems();
   }
muted hemlock
#

hmmmmmmmmmm

mossy bobcat
muted hemlock
#

you could patch a codec getter onto Value that defaults to the vanilla CODEC

mossy bobcat
#

So it would be:```json
{
"forge:whatever": { ... }
}

muted hemlock
#

no, better idea, make it extend IForgeIngredientValue, which has the abstract codec getter

mossy bobcat
#

And they'd all have just one field, like vanilla

dull bison
muted hemlock
#

actually, bad idea, nvm

mossy bobcat
dull bison
#

it's just a getter for a collection of ItemStack

mossy bobcat
#

But I believe delegation based on a field name shouldn't be too hard and that would solve the overlap issue

fallow sundial
#

mhh ["#planks", ["#planks", "oak_planks"]] gotta love lists

dull bison
#

this is how the tag value works

      @Override
      public Collection<ItemStack> getItems() {
         List<ItemStack> list = Lists.newArrayList();

         for(Holder<Item> holder : BuiltInRegistries.ITEM.getTagOrEmpty(this.tag)) {
            list.add(new ItemStack(holder));
         }

         if (list.size() == 0) {
            list.add(new ItemStack(net.minecraft.world.level.block.Blocks.BARRIER).setHoverName(net.minecraft.network.chat.Component.literal("Empty Tag: " + this.tag.location())));
         }
         return list;
      }
mossy bobcat
#

Oh god this just gets worse with more looking at it doesn't it

dull bison
#

this is the test method

   public boolean test(@Nullable ItemStack p_43914_) {
      if (p_43914_ == null) {
         return false;
      } else if (this.isEmpty()) {
         return p_43914_.isEmpty();
      } else {
         for(ItemStack itemstack : this.getItems()) {
            if (areStacksEqual(itemstack, p_43914_)) {
               return true;
            }
         }

         return false;
      }
   }
mental carbon
#

Which is why I am still for the explicit route

#

Because the vanilla system is JANK AF

#

And for me personally I would never touch it with a 10 foot barpole

#

Additionally

#

If a codec is used to write it

#

(Which it is by default)

#

It will never output the array version of the compound ingredient

#

It will always output the object variant

mossy bobcat
dull bison
#

actually it will prefer the array version atm

#

should I change that order

mossy bobcat
#

I'd say have two different codecs

#

Have the normal one used for delegated stuff be not the array version

#

Oh wait that wouldn't work nevermind...

#

Gah, that's annoying

mental carbon
mental carbon
dull bison
#

so writing will always prefer the array version

mental carbon
#

The vanilla list does not allow other infredients

#

Yet if you specify that you want a compound ingredient that means that you values array now supports ingredients on top of items and values

#

In other words

#

If the datapack maker wants to combine different ingredients together he just simply needs to day so

mossy bobcat
mental carbon
#

And if he does not say so we assume vanilla logic, meaning no ingredients allowed

mental carbon
#

So I would say put those issues back in the bottle for now

mossy bobcat
#

Will the average datapack maker understand the difference between "ingredient" and "ingredient value"? No. From the perspective of the JSON they're equivalent in vanilla

mental carbon
mossy bobcat
#

For the first, and the second should reflect the first in that regard

#

Because this should be designed from a JSON first perspective

#

Or rather, in vanilla, "ingredient" is "object or list" and "ingredient value" is "list", so ingredient values look like a subset of ingredients defined by being objects. In other words - if I've got in ingredient in JSON, and it's an object, I should be able to treat it like a vanilla ingredient value because that's the defining characteristic of a ingredient value from the JSON perspective

mental carbon
#

For all he knows is that for a custom ingredient type, he needs the object format with the type:

#

Because that is what is documented everywhere

mossy bobcat
#

Its the fact that it's a JSON object

mental carbon
#

Yeah

#

So why would an array, now all of a suddon behave differently?

mossy bobcat
#

This is really the core of what I'm saying: #1136320550168436798 message

mental carbon
#

But you know what

#

I don't really care enough for this hill to die on it to be honest

#

I just brought it up

#

Because it is not standard procedure really

mossy bobcat
#

Basically, in vanilla, the way I tell if something can be put in an ingredient array is "it's a JSON object and a valid ingredient". That should hold with forge too

#

So, if something is a JSON object, and a valid ingredient, it should be able to be placed in the list style ingredients. Forge's custom ingredient types satisfy those conditions

#

And thus should be able to be placed in the list style ingredients without further changes, from the JSON perspective

mental carbon
#

I mean you are right..... Technically

#

But it is still a bit of a mess IMHO

mossy bobcat
#

Yeah, ingredients are in general

mental carbon
#

Okey I patched out ItemStackFilter

dull bison
mental carbon
#

Yeah

#

It created 150+ hunks

#

And I made that same logic with 3

#

I just did not come up with the idea myself sadly

#

Shrimp and covers dis

fallow sundial
#

CodecCapableItemPredicate that's a mouthful

mental carbon
#

Yep

#

But it works

#

I still don't know if I need an EXC if I change a field like that

#

I don't think so

#

Its syntax is the same

#

Its name is the same

#

Just its init code changed

#

Which does not matter I think

fallow sundial
#

may I suggest SerializableItemPredicate

mental carbon
#

right now I feel uncomfortable with serializable as the prefix for thigns that have a codec

#

I know it is their purpose

#

But previously that ment a custom serializer

#

And people can interpret the wrong stuff into it

fallow sundial
mental carbon
#

Especially when not coming from 1.18 and above

#

But I like the name none the less

#

It is shorter

#

So meh?

#

Lets just use it?

#

What do you say?

#

I am for sure not going to stub my toe on it

mossy bobcat
#

CodecAwareItemPredicate

#

Shorter than Serializable even

fallow sundial
#

Serializable is the more correct word but yeah, aware works too as it's lighter than capable

mental carbon
#

Coolio

mossy bobcat
dull bison
#

orion you do know that ExtraCodecs.withAlternative will never use the second one for writing. Even if the first fails on writing it just errors instead of trying the second. That's why I created the ForgeExtraCodecs version

mental carbon
#

Okey I likely need to fix that then

#

Where did I use the vanilla codec?

dull bison
#

it uses the either codec under the hood and for writing just does Either.left

mental carbon
#

Yeah I just relaised

#

But did I use it in a place that matters?

dull bison
#

ItemPredicate

#

it will always use the CodecCapableItemPredicate.CODEC.xmap for writing and never the vanilla fallback

mental carbon
#

Yeah that is fine

#

It has no direct concequence on the functioning of the codec

dull bison
#

1.20.2-pre4 24 reject files

#
  • RecipeBookComponent
  • WorldOpenFlows
  • ChatScreen
  • Screen
  • ClientHandshakePacketListenerImpl
  • ClientLevel
  • ClientPacketListener
  • ServerData
  • TextureAtlas
  • Minecraft
  • BoatDispenseItemBehavior
  • DispenseItemBehavior
  • Main
  • CompoundTag
  • NbtAccounter
  • NbtIo
  • StringTag
  • ServerPlayer
  • Hoglin
  • ThrownEnderpearl
  • MerchantOffer
  • MinecartItem
  • DropperBlock
  • LevelStorageSource
mental carbon
#

We are currently on hold for pre4

#

Please do not port further at the moment

kindred fractal
#

Switching to patches in mojmap? 😍

stray scroll
#

i'd be :D for that

mental carbon
#

And at runtime

#

Basically everywhere

#

Removes some mapping options

#

Yarn is now not possible anymore

#

But parchment still is

kindred fractal
#

Cool!

#

Great news

inland mesa
#

JsonThings scripting here we go (please read with mario voice not chris pratt voice)

mossy bobcat
#

That means we can dump runtime remapping in GML, which will be nice

true pivot
mental carbon
#

Sorry

#

We have decided to basically remove 90% of the complexity from the pipeline

true pivot
#

that's really bad news for ports of fabric mods

#

although I suppose the ability to run in dev with another mappings set can be implemented externally

arctic sphinx
#

Not really? Yarn was only half possible anyways with the fact that Class Names where fixed as MojMaps

mental carbon
#

^

true pivot
#

archloom had logic to correct that

arctic sphinx
#

Archloom is based on loom

prime quest
#

There's no reason archloom must also remove mappings channels like we did

arctic sphinx
#

It shouldn't be majorly effected by what we do? Allthough the change to runtime mojmaps will probably require an update

prime quest
#

If people already use it, they can continue to use it. right?

true pivot
#

I guess it just means mojmap should be used as the intermediary instead of SRG

mental carbon
true pivot
#

Ok yeah this isn't as bad as it sounded initially

mental carbon
#

But it gets us patches in mojmap

#

Runtime mojmap

#

Etc

#

No more reobf

#

The jar you build is simply ran

arctic sphinx
#

Removes complexity, removes confusion between potential NeoSrg and ForgeSrg, etc

#

There's, at least what I think most of the team would consider, little downsides for major positives

rich void
#

RC1 @snapshotalarm

shadow widget
#

wee woo wee woo

mental carbon
#

Shit

rich void
mossy bobcat
#

...welp. Time to remember what else I wanted to try to write in time for the 1.20.2 BC window I guess

#

Good luck to the porting folks; I don't envy trying to figure that out

arctic sphinx
#

(Although think I missed pinging for Pre4)

prime quest
#

Yeah it takes a good moment

#

They seem to have two different processes for releasing the piston-meta file and the blog post

#

So they can be out of sync by up to an hour

rich void
inland mesa
#

rc1 :o

prime quest
#

RC heck

arctic sphinx
#

Also can we start handing out that role yet for those who want it?

prime quest
#

I think so

arctic sphinx
#

And mdcfe is up to date

prime quest
#

Let me check

#

Damnit. Connection broken, give it some time

muted hemlock
prime quest
#

Yes

muted hemlock
arctic sphinx
mental carbon
prime quest
arctic sphinx
#

Ah

mossy bobcat
prime quest
#

Imagine being able to debug modpacks

mossy bobcat
#

Imagine being able to reproduce forge bugs in a dev environment

tranquil salmon
#

I may need to rerun the update for neoform

mental carbon
#

I am checking if my patch to jammer now works 😄

dull bison
#

ping me when I can start porting again

mental carbon
#

It will take me a while 😄

#

@dull bison Actually, if you could do the following:

  1. Create a new branch named 1.20.2-pre3 from the ucrrent working state.
  2. Then start the update, using #1136320550168436798 message as a guide, note, you need to delete the projects folders first. Additionally the version of neoform that you need to use is: 1.20.2-pre4-20230913.163358
#

Due note, perform the update again on the normal working branch for 1.20.2

#

I just want to keep a nice lookup backup behind in the private repo

mossy bobcat
frail oriole
#

i admit that when asked, i recommended when this first came up, to REMOVE the intermediate mapping

#

mojmaps at runtime was always my preferred endstate.

arctic sphinx
frail oriole
#

in general, mods themselves will be in compliance with the license. the only ones "at risk" are people such as ourselves, who are doing the dirty with the files. but are we manipulating files (a potential copyright violation) or the data from the file (never a copyright violation). It's such a stupid fine line.

#

is the tsrg a derivative work of mojmap, or not?

#

if you consider mojmap a dataset, then there is no inherent copyright. data can't be copyrighted.

fallow sundial
#

tsrg is a file format smh

dull bison
mental carbon
#

Cause that was the pre1 port we just finifhed

dull bison
#

ok 1.20.2-pre1 is pushed with the current state

#

hah a single rejected hunk for pre4

mental carbon
dull bison
#

it failed on one hunk but CompoundTag and NbtIo changed a lot

mental carbon
#

Oeh

#

If you want you can push that state and we do it together

dull bison
#

I'll fix some other compile errors first

fallow sundial
#

we also need to decide on versioning before releasing 1.20.2

mental carbon
#

For sure

#

But i need to make massive modiciations to the buildscript

#

Mostly pressing the DEL key

#

But still

keen hearth
#

YEET the remapping!

dull bison
#

we have 2-4 days for that concern (I think we're not gonna make a same day release this time)

keen hearth
#

my life will be complete when genPatches has no dependencies, oh the development productivity!

mental carbon
dull bison
#

am committing rn

mental carbon
#

Noice

fallow sundial
#

@dull bison for the future, you push as soon as you update the MC source, and you fix stuff in a later commit

#

so there's a diff of the changes / fixes

mental carbon
#

Okey here we go

#

Lets rip out everything related to mappings XD

keen hearth
#

*kinda

quiet talon
#

and mixin won't need refmaps anymore

vague otter
#

will there be a tool for updating ATs? [though hopefully most people will have the moj name commented, which would allow it to be updated with a simple regex]

keen hearth
#

I don't see why not. Someone should port out the AT magic thats in the Forge buildSrc plugin, into FG for users to use. Validating and such

mental carbon
#

Okey range maps are gone

stray scroll
#

I tried in the past; I think I fell into the problem of how to get an MC artifact that doesn't have the ATs applied, to check the ATs against, since otherwise you can't do a check for when an AT is making public something already public, or something like that

#

been a while since I did that, so I may be wrong thinkies

keen hearth
#

ugh, ofc its not easy

#

because FG

#

there should be some black magic somewhere you can use to grab the renamed but untransformed artifact

#

as its in the user cache, the file does exist

#

Personally AT groups are just stupid

vague otter
#

so uh on the subject of runtime mojmap and ATs

#

right now we don't have a problem if one mod makes a field public and another mod makes a subclass with a field with the same name, because the field isn't really called that

#

what are we gonna do about that? or is it still not a problem because bytecode says the declaring class?

quiet talon
#

ye

vague otter
#

[it is occasionally a problem in dev iirc]

quiet talon
#

You can have multiple fields and methods with the same name in bytecode but not in source

#

iirc you can actually (in bytecode) have different fields with the same name on the same class, as long as the types are different

stray scroll
#

here's what the JVMS (18) has to say on that, per section 4.5,

No two fields in one class file may have the same name and descriptor (§4.3.2).

prime quest
#

fields.

stray scroll
#

substantially similar paragraph in section 4.6

No two methods in one class file may have the same name and descriptor (§4.3.3).

prime quest
#

Interesting

keen hearth
#

You can hide fields, yes

quiet talon
#

Yeah those check out as bytecode rules

mossy bobcat
quiet talon
#

Bytecode allows this while source does not

public A get();
public B get();
mossy bobcat
#

Yep. Bytecode relies on that in fact for bridge methods of certain types

keen hearth
#

bytecode uses name+desc for unique key, on both fields and methods

mossy bobcat
#

As in, a class implementing Supplier<String> will have a String get() and an Object get() in bytecode

stray scroll
keen hearth
#

yay bridge methods ._.

#

speaking of bytecode trivia, did you know that record accessors/hashcode/equals are the only methods generated by javac which do not have the synthetic flag

stray scroll
#

👀

#

interesting thinkies

#

i wonder if that's a bug or a feature

keen hearth
#

bug

#

technecally a JLS spec violation

#

cant cite, but ifaik the jls specifies that all members that do not exist in source must have the synthetic flag

mossy bobcat
#

thinkies those don't have the synthetic flag? Weird

keen hearth
#

yep, had to special case in my decompiler, becuase, javac never makes it easy ._.

stray scroll
#

looked online, and it seems it's noted in JLS 13.1 (the cinary compatibility chapter)

  1. A construct emitted by a Java compiler must be marked as synthetic if it does not correspond to a construct declared explicitly or implicitly in source code, unless the emitted construct is a class initialization method (JVMS §2.9).
keen hearth
#

there you go

prime quest
#

I mean, record implies having equals, hash and accessors

#

That's literally the purpose of records

#

Seems like it's in spec to me.

fallow sundial
#

construct declared explicitly or implicitly in source code

prime quest
#

That's what I said

fallow sundial
#

the record methods are implicit

keen hearth
#

well, thats dumb

#

they should be synthetic

#

would make my life easier

fallow sundial
#

well they don't have to

#

so

#

¯_(ツ)_/¯

keen hearth
#

i can argue that the accessors are implicit due to the components, but the equals/hash/tostring, nah

stray scroll
#

actually, this SO page I'm reading raises that same argument, too
with a reference to the JLS as well
JLS 8.10.3

A record class provides implementations of all the abstract methods declared in class Record. For each of the following methods, if a record class R does not explicitly declare a method with the same modifiers, name, and signature (§8.4.2), then the method is implicitly declared as follows:
[... definitions for equals, hashCode, toString...]

If a record class has a record component for which an accessor method is not declared explicitly, then an accessor method for that record component is declared implicitly, with the following properties:
[...]

fallow sundial
frail oriole
# vague otter what are we gonna do about that? or is it still not a problem because bytecode s...

it's surprisingly not a runtime problem - the refs are always hardcoded - so the bytecode always knows what you're talking about. There's not really a "virtual" lookup table (there is, but it has special calling semantics). so in general, if you add a new name in a subclass, it'll just be ignored, invisible. this is actually already a "problem" in modding, shadowing fields and methods and stuff. mixin has a whole slew of special stuff to try and help handle it.

stray scroll
#

oh hey, even the JVMS18 explicitly excludes those record members from being marked as synthetic (whether by Attribute or by access flag)
JVMS section 4.7.8 (the Synthetic Attribute)

[...] A class member that does not appear in the source code must be marked using a Synthetic attribute, or else it must have its ACC_SYNTHETIC flag set. The only exceptions to this requirement are compiler-generated members which are not considered implementation artifacts, namely:

  • an instance initialization method representing a default constructor of the Java programming language (§2.9.1)
  • a class or interface initialization method (§2.9.2)
  • the implicitly declared members of enum and record classes (JLS §8.9.3, JLS §8.10.3)
vague otter
#

now i'm curious if, if you make a private method protected (or public) and override it in a subclass, will its callers actually call your override

#

i'd always assumed so

keen hearth
#

It does actually do a virtual lookup super lookup for fields:

public class EggSample {

    public String getString(B b) {
        return b.aField;
    }

    public static class A {
        public String aField;
    }

    public static class B extends A{

    }
}

  // access flags 0x1
  public getString(LEggSample$B;)Ljava/lang/String;
   L0
    LINENUMBER 7 L0
    ALOAD 1
    GETFIELD EggSample$B.aField : Ljava/lang/String;
    ARETURN
   L1
mossy bobcat
#

Or rather the one it's compiled by

#

On new ones, yes

#

On old ones, it used INVOKESPECIAL for private methods, so no unless you have a tool to transform those to INVOKEVIRTUAL

lean warren
#

Makes me wonder how ATs handled that then since a significant part of their use is to allow overriding previously private methods and that always worked AFAIK.
What does "old Java version" refer to in this case?

keen hearth
#

iirc bellow 1.6

stray scroll
#

ATs transform invokespecials to invokevirtuals when they refer to a previously-private (transformed into non-private) method, iirc

keen hearth
#

somewhere around there ACC_SUPER was enabled by default for classes and INVOKESPECIAL is now just to target specific implementations (skipping virtual table)

mossy bobcat
#

That just looks in super classes or interfaces

#

A virtual lookup looks in subclasses

keen hearth
#

right, wrong word

mossy bobcat
#

But yeah, it checks inherited stuff

lean warren
keen hearth
#

you knew what i meant and i knew what i mean :)

mossy bobcat
#

Abstracting a field to a superclass is not a binary breaking change, luckily

mossy bobcat
stray scroll
# keen hearth iirc bellow 1.6

JVMS18, section 4.1

In Java SE 8 and above, the Java Virtual Machine considers the ACC_SUPER flag to be set in every class file, regardless of the actual value of the flag in the class file and the version of the class file.

keen hearth
#

j8, wow

#

thought it was 6

mossy bobcat
#

Note that you can still use INVOKESPECIAL for private methods by using the -XDdisableVirtualizedPrivateInvoke flag

keen hearth
#

huh, wild

mossy bobcat
#

Though it will still use INVOKEVIRTUAL in cases where nested classes are involved I believe

keen hearth
#

iirc, lambda generated classes use IVNS, from an inner-class to call the private lambda impls

mossy bobcat
#

Yep, and nested classes

keen hearth
#

but i think INVS is just a semantic there, you could use INVV

mossy bobcat
#

Well, other way around

#

Lambdas and nested classes need INVV

#

They can't use INVS, I believe

keen hearth
#

hmm

#

InnerClassLambdaMetaFactory uses INVS

mossy bobcat
#

The use of INVV for private methods happened at the same time as nest member stuff getting added

stray scroll
keen hearth
frail oriole
lean warren
#

Speaking of things the runtime doesn't give a shit about: you can AT record elements or otherwise inject non-final fields into records and the JVM doesn't care that they technically break the entire contract of a record 😄

sly anvil
frail oriole
#

yeah. we should probably protect that, because that could be quite breaking

mossy bobcat
#

Now, this does lead to a potential runtime issue with ATs, especially in Mojmaps where you don't have srg names helping you:

  • Minecraft defines method a in class A which is private
  • mod extends that class with its own method a, which is public or whatever, and is unaware that the super method exists as it's private
  • ...another mod ATs the super method
    that can cause issues
fallow sundial
mossy bobcat
fallow sundial
#

and tbh i'd argue that if mixin can inject fields into records it's a feature and not a bug 😛

mossy bobcat
#

But yeah, the actual lambda invocation is INDY

lean warren
frail oriole
sly anvil
mossy bobcat
frail oriole
#

test case please

mossy bobcat
stray scroll
#

to echo cpw, I would like to see a test case too

mossy bobcat
#

I can't actually do that without runtime Mojmaps though

frail oriole
#

you can build mocks pretty easily.

mossy bobcat
#

Heck wait I can just use srg names

stray scroll
#

perhaps a light env with the AT engine?

keen hearth
#

you can do it with a method that just so happens to have a method name thats an srg though

mossy bobcat
#

It'll look weird but it'll still work

keen hearth
#

to 'simulate' runtime moj

mossy bobcat
keen hearth
#

in practice, i dont think it will actually occurr, the likely hood of someone implementing a function with the same signature and somehow turning into a virtual lookup will be very low

mossy bobcat
keen hearth
#

especially on the classes that mods commonly implement, most functions are public or static private/public

mossy bobcat
#

Because if so that would help us see how likely it is someone overlaps with one of those just based on whether they're in things you tend to extend and how generic the name is

fallow sundial
#

no not right now

frail oriole
#

i don't think this is a real issue. that's why i want to see what you get in a mock. i think you'll find that the "ATed" subclass method is just ignored.

keen hearth
# 
    public static class MojangClassA {

        private String getMyString() {
            return "MojangClassA";
        }

        public void doThing() {
            System.out.println(getMyString());
        }
    }

    public static class ModClassB extends MojangClassA {

        public String getMyString() {
            return "ModClassB";
        }

        public void doThing() {
            super.doThing();
            System.out.println(getMyString());
        }
    }

will print:

MojangClassA
ModClassB
#

If we apply public MojangClassA.getMyString ()Ljava/lang/String;

#

it will print:

ModClassB
ModClassB
#

thats the case luke is describing

tranquil salmon
mossy bobcat
#

Got around to testing it:
Install just firstmod. Play the game. Give yourself a firstmot:example_item (from the tools tab not the creative menu); you can right click it to "blow" it without issue (it's an instrument item, like goat horns). Now install just secondmod. Nothing much should happen except some logging during game start. Now install both, give yourself that item (once again from the creative menu), and attempt to blow it:

java.lang.ClassCastException: class net.minecraft.resources.ResourceLocation cannot be cast to class net.minecraft.core.Holder
@cpw example of this; it uses SRG names so the bit in firstmod looks a bit ugly; obviously would be cleaner in mojmaps. That was just the first private method I could find and easily write a test mod for - I highly suspect that any issues we see with this will come out of Entity and company, especially some of the subclasses, which just have a lot of methods. Not sure how much of an issue this is, but if it's a real worry, we could make FG run a check on jars and error on any classes that contain methods that have the same signature as a private superclass method

📎 secondmod-1.0.0.jar 📎 firstmod-1.0.0.jar
#

... @frail oriole cause my ping didn't want to work

#

firstmod is the one with the badly named method, secondmod has the AT

#

The class I did this with is InstrumentItem

#

Let me bundle up the source quickly and dump that here too

#

The worst part is that the stack trace doesn't mention either of the mods in question, at any point in time whatsoever

#

TL;DR it's easy to come up with a case where this could break; I expect such cases to be very rare but in the huge number of combinations of different mods out there maybe we'll see one and this'll help us figure out what an otherwise unintelligible stack trace means

#

But if anyone asks why I prefer accessors over ATs I will now point them at this

#

This sort of bug, if it actually pops up, would be very hard to diagnose; my example here causes a crash but this is also the sort of thing that could have, like, silent behavioural changes. I'm actually pretty tempted to see how hard it would be to make FG enforce stuff so this isn't possible

#

I'm also going to build a small tool, if I can, to make sure there's nothing in vanilla where vanilla potentially does this to itself

frail oriole
#

Ok. So it's as I said. The override is invisible in runtime and could cause secondary issues but isn't a direct crash. Checking is obviously worth it during mod building. But runtime checks are gonna be expensive and slow

#

This could be an additional issue with mojmaps vs srg, but it's not really a huge change

mossy bobcat
frail oriole
#

Yeah, if you can figure it out. I think it's just something to be cognizant of.

mossy bobcat
#

Could probably be embedded in the remapping system?

#

I'll have to poke the relevant bits. I'll probably wait till NGNext exists unless it looks like it's going to be separate from that chunk of stuff

#

My biggest worry isn't "this'll happen all the time", but "if this does happen it's borderline impossible to diagnose in basically every case it'll happen in, whether it's a crash or a silent issue"

#

Because there's... nothing, at all, to clue you in on what the culprit is 9 times out of 10, with the possible exception of "an item from this mod caused it" which is only really relevant in cases like this where it's a nice, obvious item

mossy bobcat
#

But yeah, I suspect this sort of check could be embedded in remapping, I just have to see how that system works

#

...fuck nevermind we won't be re-obfing mods anymore will we, which is actually great, just bad for this particular thing

mental carbon
#

I am also nuking the naming services

#

Cause they are a massive overhead on the naming channels

#

Lock us in into a legacy format

#

And are not usefull anymore

mossy bobcat
#

Hmm. As the gradle expert, do you have any ideas as to how plausible such a compile time check would be?

lean warren
mental carbon
#

on what*

#

specifically**

#

Jesus

#

My typing

mossy bobcat
mental carbon
#

Make a task, run an analysis with ASM

#

That is likely the fastest way to do it

mossy bobcat
#

That's what I figured; I was wondeirng if NG already did any sort of ASM checks/modification passes that would be easy to hook into for stuff like this

mental carbon
#

No we do not

#

On purpose

mental carbon
#

AT'ing private methods which are in none final classes is not allowed by definition

#

Because of that exact reason

mossy bobcat
#

If they're ATed they're no longer private

mental carbon
#

Yeah

mossy bobcat
#

So the check won't fail for the override of them

mental carbon
#

But any inherting class could have the same method and signature in them

#

As far as I know this is a problem at runtime

#

Cause it can create completely unintended behaviour

mossy bobcat
#

Yes it can. Hence why you want to run that check

mental carbon
#

Due to one mod making a method protected

mossy bobcat
#

See: #1136320550168436798 message

mental carbon
#

I know, but that does not need to be a check

#

Simply prevent the AT from being applied at runtime

#

And show a nice error message

#

And bob is your uncle

mossy bobcat
#

It's not an issue with SRG but it'll be an issue with mojmaps

mossy bobcat
#

*efficiently, that is

#

You load a vanilla class with an AT to a private method

#

How do I know whether that AT is safe to apply?

mental carbon
#

PR it to accesstransformer

#

You don't

mossy bobcat
#

Exactly. It has to be a compile time check.

mental carbon
#

Any attempt to AT a private method in a none final class is by definition not safe

#

Not even your compile time check guards against that

tranquil salmon
mossy bobcat
#

It kinda does though

#

The compile time check works fine, with the exception of mods that bypass the compile time check

#

Such a compile time check would prevent this issue

lean warren
mental carbon
#

So I did not

mental carbon
#

Period

#

End of discussion, because no matter the compile time a check

mossy bobcat
#

Once again, I don't see why not?

mental carbon
#

Because there is no way to protect the runtime from undefined behaviour

mossy bobcat
#

Sure? That's never the goal

#

The goal is to prevent it from happening accidentally

mental carbon
#

The simplest solution

tranquil salmon
#

The constructor parameters need to be remapped to the field names provided by mojmap

mossy bobcat
#

If you're bypassing the compile time check...

#

That's your own fault

mental carbon
#

They are a, b, c, d, e, f, g, h etc

#

Or do you mean their actuall names

#

Ahhh

#

Hold on then

mossy bobcat
#

And it is possible, with a compile time check, to make it so that the only possible issues come from mods that bypass the compile time check. That is trivial

mental carbon
#

Will patch that

mossy bobcat
#

Well, by "trivial" I mean the "how to do it" is. The actual doing would be an ASM run over the jar

mental carbon
mossy bobcat
kindred hatchBOT
#
lukebemish

[Reference ➤ ](#1136320550168436798 message)Got around to testing it:
Install just firstmod. Play the game. Give yourself a firstmot:example_item (from the tools tab not the creative menu); you can right click it to "blow" it without issue (it's an instrument item, like goat horns). Now install just secondmod. Nothing much should happen except some logging during game start. Now install both, give yourself that item (once again from the creative menu), and attempt to blow it:

java.lang.ClassCastException: class net.minecraft.resources.ResourceLocation cannot be cast to class net.minecraft.core.Holder
@cpw example of this; it uses SRG names so the bit in firstmod looks a bit ugly; obviously would be cleaner in mojmaps. That was just the first private method I could find and easily write a test mod for - I highly suspect that any issues we see with this will come out of Entity and company, especially some of the subclasses, which just have a lot of methods. Not sure how much of an issue this is, but if it's a real worry, we could make FG run a check on jars and error on any classes that contain methods that have the same signature as a private superclass method

mental carbon
#

Write down in a sentence or two what the condition is

#

That you are checking for

kindred hatchBOT
#
lukebemish

[Reference ➤ ](#1136320550168436798 message)Now, this does lead to a potential runtime issue with ATs, especially in Mojmaps where you don't have srg names helping you:

  • Minecraft defines method a in class A which is private
  • mod extends that class with its own method a, which is public or whatever, and is unaware that the super method exists as it's private
  • ...another mod ATs the super method
    that can cause issues
mental carbon
#

Yeah

mossy bobcat
#

You can do that at compile time

mental carbon
#

So you want a validator for ATs?

mossy bobcat
#

You don't need to know that the other mod with the AT exists

mossy bobcat
#

This doesn't care about ATs

mental carbon
#

Do you want a simple check that runs through all ATs and barfs if you add one for a private method in a none final class?

mossy bobcat
#

It's validating classes that re-use names/descriptors that could be ATed in vanilla

mossy bobcat
mental carbon
mossy bobcat
#

This checker never looks at ATs

mental carbon
#

That behaviour is JVM valid!

mossy bobcat
#

But not mod valid

mental carbon
#

The AT is the magic runtime tool

mossy bobcat
#

And mods should adapt to it

mental carbon
#

It should be coded as such that it never breaks the JVM

#

No

mossy bobcat
#

It is impossible to code it as such that it never breaks the JVM

mental carbon
#

Mods which are doing legally valid java with forge, without a single coremod or AT

#

Should NEVER have to adapt their code

#

To mods that work with ATs, mixins or coremods

#

That is my opinion

#

And I refuse to help people which are trying to achieve otherwise

#

Simply said if you want to do this, fine

#

Make a tool that checks the compiled jars

#

It is trivial with ASM to do

#

Create a task that invokes the tool with the relevant jars as input

#

And throws an exception if the tools finds errors

lean warren
mossy bobcat
#

I'm a bit confused what the alternative is to a compile time check? If this issue happens at runtime it is impossible to diagnose in many cases

frail oriole
#

that's kinda my point too orion. this is a thing people will do sometimes.

#

you have to build a specifically pathological case to make it break the way luke did.

mossy bobcat
#

Like, this is a thing that could, genuinely, happen very easily with the switch to mojmaps

frail oriole
#

it's a thing that could happen, yes.

#

but not without compiler warnings

mental carbon
mossy bobcat
#

If we're switching to runtime mojmaps, we have a responsibility to make sure that it doesn't because of the severity of it if it does happen, and how difficult it is to diagnose

frail oriole
#

because don't forget, the modder sees the ATed MC and neo.

#

if they apply an AT, they see it.

mossy bobcat
#

We're talking two different mods

mental carbon
#

The problem is two mods cpw

frail oriole
#

ok

mental carbon
#

One that is perfectly legally using java

#

And forge

mossy bobcat
frail oriole
#

both are perfectly legal java

mental carbon
#

And one that is using an AT to do something that is normally not legal in java

#

Because the method is private

frail oriole
#

and we cannot implement a runtime check, because that's stupidly expensive. inheritance checks are

mental carbon
#

IMHO, the mod that is doing nothing wrong, should not be the one that needs to be adapted.

true pivot
#

personally i'd argue its on the mod with the AT to validate that what they're doing is unlikely to break things

#

but in practice the average modder probably lacks an understanding of bytecode at that level 🙃

mental carbon
#

That is a trivial check to make

#

And costs little to no CPU time

mossy bobcat
mental carbon
#

Hell we could technically even show an error during project setup

#

And not allow the AT at all

#

If we just fail the access transformer tool with such an error message

mossy bobcat
#

So you're saying "butcher ATs"

true pivot
mental carbon
true pivot
#

the question is: does this occur often enough in practice that we care

mental carbon
#

I don't know

mossy bobcat
mental carbon
#

But I don't think that a completely legal mod, needs to adapt and change its code, because another mod is doing something agains the java compiler rules

mossy bobcat
frail oriole
mossy bobcat
#

Like, logs don't implicate either mod involved, etc

mental carbon
#

It can make protected methods public

mossy bobcat
true pivot
#

is it not possible to warn (not crash) for this at runtime in the classloader?

mental carbon
#

It just can do anything to private methods

frail oriole
mental carbon
mossy bobcat
mental carbon
#

Mods without ATS are just more perfectly valid

frail oriole
mental carbon
true pivot
frail oriole
#

IMO we should just ignore it

mental carbon
#

I would ignore it too

frail oriole
#

inheritance checks are EXPENSIVE.

#

trust me. the speed up when we removed a few was NOTICEABLE

mossy bobcat
# true pivot i'm skeptical that the check would be that costly

Okay, so, you're loading a class. You need to check every super class and super interface of the class for any instance methods that share names with methods of the class being loaded, check if an AT has been applied to any of those fields from private to whatever, and if it has error/warn/whatever. For every class you load.

mental carbon
#

I mean if we add this compile check

true pivot
mossy bobcat
#

Yes

frail oriole
#

btw: you have to do this while classloading, which means you can easily get into circularity fun as well

#

it's NOT cheap, and i will veto any solution that requires doing this.

mossy bobcat
#

Let me put it this way orion: the number of cases where this occurs at runtime should be comparable to the number of mods that would be affected by my compile time check. If you're not worreid about this happening at runtime, you shouldn't care about the compile time check because it'll never catch anything

frail oriole
#

IMO, this is NOT a serious issue. It's a possible problem, YES. but it's one in a crowded field of thousands.

mental carbon
#

And a modder does add his own version of the method which is private in the super class, which is completely legal in java to do, there is nothing wrong with that. And we will then tell that modder: Great you are using forge and java to its fullest potential, but sorry for you, there are modders which do something that is normally illegal in java and you break their mods, as such you are not allowed to do this?!?

mossy bobcat
#

It's a problem that we won't notice if its happening

frail oriole
#

won't we? we know pretty much everything here.

true pivot
frail oriole
#

it might be difficult to diagnose directly.

mossy bobcat
mental carbon
#

We do nothing at compile time

#

On purpose

#

By design

#

If the compiler allows it

#

It is fair game

frail oriole
#

we have three choices:

  1. ignore it. if it becomes an issue, we might have to think again.
  2. drop mojmap. We break the whole world when we create conflicting srg.
  3. never release 20.2.
#

pick

#

now

mental carbon
#

I vote for 1)

frail oriole
#

me too orion, me too

#

luke seems to think 2 or 3 are better.

mossy bobcat
#

...that's not at all what I have been saying

#

And frankly I'm a bit insulted that you'd imply that

frail oriole
#

isn't it. it's what i'm hearing.

#

you think this is a serious issue with mojmap

mossy bobcat
#

I'm saying that we should build a compile time checker to prevent this issue

frail oriole
#

i don't

#

i think it's a "thing to be aware of". nothing more.

mossy bobcat
#

I think that the switch to mojmaps is essential

true pivot
#

I remain skeptical that option 4 (runtime check) is costly

mental carbon
#

Well no cpw, he argues taht we need to prevent modders from making private methods with the same name and signature as private method in their super type

frail oriole
#

runtime check is too costly without evidence of a real issue

#

which is effectively "remove AT"

mental carbon
#

NO

#

It is actually

#

Normal modder: Adapt to modder with AT

frail oriole
#

hmmm

mental carbon
#

It actually does not complain about the AT

frail oriole
#

yeah

mossy bobcat
frail oriole
#

it tells off the normal modder, not the problematic modder.

true pivot
#

since when are ATs problematic

frail oriole
#

that's the problem tho. the possible actions of one modder affect the entire space.

mental carbon
#

It barfs at the normal modder, which is doing something completely legal, that he is potentially, going to break a users runtime if the user installs a problematic mod

frail oriole
#

and you're saying that we need to warn the entire space of modders because of that.

mental carbon
#

They are currently what the issue is

frail oriole
#

it's not about problematic or not.

mossy bobcat
#

And frankly I'm tired of you guys making straw man arguments out of this. I'm not saying that this is some huge flaw with ATs, and I'm not saying it should hold up mojmaps or whatever, I'm just saying that this is an issue that is subtle enough and severe enough that we should proactively prevent it

true pivot
#

I see

mental carbon
#

Because they circumvent the normal protections of the client

frail oriole
#

it's that we're wanting to bake in a warning to everyone that "someone could break this"

mental carbon
frail oriole
#

may as well just make that warning output all the time. "warning, your method might be mixined at runtime!"

#

that's not helpful

mental carbon
#

I mean I would be fine with a warning at most

#

But for sure not an error

frail oriole
#

it's part of the territory of modding.

mossy bobcat
#

Here's an idea: the compile time checker, instead of erroring, writes the potentially problematic stuff to a file

frail oriole
#

i would probably want to think twice even as a "normal" modder if i am overriding a mojang class, and use the same method name (private or not)

mossy bobcat
#

That file is bundled into the jar

#

And forge logs its contents at runtime

#

That way, the information is there

#

So the problem, if it occurs, isn't as "hidden"

#

Heck, forge could match up ATs and contents of that file at runtime

mental carbon
#

Actually luke that might not be a bad idea

#

Something to the extend of the annotation metadata

#

Just for those kinds of inheritted methods

frail oriole
#

a compilation logger might be good

mossy bobcat
#

You've got a compile time check, that makes the runtime check cheap

frail oriole
#

but it's gonna be for everything

true pivot
#

this sounds like it eliminates most overhead of the check

frail oriole
#

because it's NOT JUST AT

mossy bobcat
#

You can log a warning in dev, and an error at runtime

frail oriole
#

mixin has all the same potential problems

mossy bobcat
#

Mixin actually does not

frail oriole
#

yeah it can

mossy bobcat
#

Mixin cannot change method visibility

#

So someone has to be intentional to mess with stuff

frail oriole
#

no, but it can completely change the behaviour of an internal method

#

thus causing invisible problems 😛

true pivot
frail oriole
#

arguably so can coremods.

mossy bobcat
mossy bobcat
frail oriole
#

it's part of the ecosystem of mods

mental carbon
frail oriole
#

we log a bunch of info about ATs and coremods too 😛

mossy bobcat
#

The issue is that ATs are generally thought of as "safe" and it's not obvious that it could break stuff. Unlike, say, mixins and coremods where its really goddamn obvious

lean warren
mental carbon
#

IMHO, we should put this on hold, but luke could you do me a favor and go to the NG repo, and open an issue

mossy bobcat
#

Additionally, mixins shove info about the mods involved in stack traces