Let's talk privately

[asiekierka]

Let's talk privately

[asiekierka]

Excellent work!

Now, let's hope for a soon resolution of Duke Nukem Fabric Configs Forever.

[asiekierka]

While this is a personal statement and not an opinion necessarily shared by the devteam, I have to ask: How is it fair for us to be modifying Mojang's game but not allowing us to modify one another?

[asiekierka]

"Should" doesn't mean "we should formally block it", though. There are many things you shouldn't do but we don't ban them outright. I don't think we're in a position to be judges.

If it causes additional implementation hurdles (and ATs on mods would), that's another story.

[asiekierka]

@RedstoneParadox We're talking about Mojang, who doesn't really design the engine with modding in mind.

[asiekierka]

@Darkhax It has potential value in terms of putting slightly stronger thread safety guarantees without injecting ThreadLocals and thus breaking existing code. I'm not sure if it's practically useful, though.

Notice that all of those weaken available assumptions. This is the common motive. public weakens the assumption that, say, a method can only be accessed in one package; mutable weakens the assumptions that a field will never be written to twice; likewise, volatile weakens the assumption that the field may be stored in one core's CPU cache only without pushing it off to RAM (Without volatile, it may be pushed off to RAM at any time; with volatile, it must be after the write has been done).

I was, in essence, digging through the access flags available to classes, fields and methods. I found that synchronized/native could have had some value to methods, but those are best added with a mixin IMO.

[LemmaEOF]

FabricBlockSettings probably isn't the best example, since we depend on it for a few other things like breakByTool, drops, and dynamicBounds.

[Frontrider]

I know, but you get the gist. Maybe gamerules migt be a better example, if I recall correctly.

[kashike]

Another thing to keep in mind: constructor visibility, and extending classes with non-public constructors.

[RedstoneParadox]

I'm a firm believer that if someone sets their class to private or package-private, then we shouldn't mess with that choice; if they want to allow other people to use Mixins on their class, then they will set it to public, so I'm voting for number 2.

[RedstoneParadox]

Well, yeah, but modders should have some way to protect their stuff from outside modification.

[RedstoneParadox]

Good point.

[modmuss50]

1. Its not working as is and something extra needs to be done.

2. I think just adding them is honestly going to sort out all of the issues, one idea would be for fabric to contain the most commonly used AT's, this way you have a good chance of mods just working when you drop their source code into your IDE.
   This does slow up loading a little bit but im not sure it will be that bad?
   Pomf Yarn could also include some AT's to allow more freedom when mapping things out

3. Doesn't cover all the cases, more would still be needed

4. Having a helper class would be nice but does have some issues. I can see it getting messy quite quickly if you need to use it a lot, and what is the performance like? Slower loading is a lot better to have than lower in game performance.

[Prospector]

There's one more option. Not allow everyone to use ATs, but have a communal repo--similar to Yarn--with ATs that people who present decent cases can add their ATs to. This repo would have to be very strict about accepting PRs after a lot of reviews and input. Ideally, someone can suggest a better way to do what the person needs to do without ATs, but if not, then they get added.

I'm not saying I necessarily like this solution but it's another idea.

[asiekierka]

We considered this, but one of our cornerstones was "everyone has fair access to the same tools". Allowing only us to define ATs would go against this principle, even if it solves some of the logistics.

[Pannoniae]

Based on my limited knowledge, Approach 4 seemed the most logical to me.

[Frontrider]

I think this kind of tooling should only be used on vanilla classes because we're available to change APIs, unlike Mojang. It is nothing more than a scapegoat and should be treated as such.

Just add transformers.

[asiekierka]

Access transformer rough spec:

• Can only be used on Minecraft JAR classes. Period.

net/minecraft/block/Block.field_1234: public mutable
net/minecraft/block/Block$Builder.method_2600: protected # I'm a comment!
net/minecraft/block/Block$Builder.<init>(Lnet/minecraft/block/Material;): protected
# I'm a comment too!

(Well, in practice, you'd have class_... there, but just for the sake of example)

• Allowed keywords:
  • public - upgrades the access visibility to public,
  • protected - upgrades the access visibility to protected,
  • mutable - removes finality, if present,
  • volatile - adds the volatile access flag, if not present.
• Operations which can break other existing code (downgrading access visibility, adding finality, removing volatility...) are forbidden by Fabric, though additional keywords could be added by other implementations.

[Darkhax]

That AT spec looks good to me. I am curious about the inclusion of volatile in this list though. Do you have any examples of why it would be needed, or is it there because it could have some potential value and wouldn't be able to mess things up too badly?

[Runemoro]

This can be worked around, though: simply copy the prepared .JAR, only edit the classes which have changed, then only decompile those classes!

This might not always work. If a class contains the two methods:

public void someMethod(Object o) { ... }
private void someMethod(String o) { ... }

then making someMethod(String) public would mean that that calls to something.someMethod("abc") in other classes would now refer to someMethod(String) rather than someMethod(Object). All classes invoking the method transformed would have to be redecompiled such that a cast is added by the decompiler: something.someMethod((Object) "abc").

[2xsaiko]

No, because the actual bytecode in all the classes still calls someMethod(Ljava/lang/Object;)V

[Runemoro]

But not the source code. The source code for all classes that call the method with a string argument will now be wrong.

[2xsaiko]

Ah, I see what you mean now. Yeah, that's a problem. I thought you meant that the game's behavior will change because now the wrong methods get called, which as far as I know, won't happen (of course, it will happen if anything in the mod's source code calls one of those methods, but that's going to happen either way)

[asiekierka]

@Runemoro The source code is only for viewing in the IDE; I think this cost can be paid, so to say, especially as such cases are fairly uncommon regardless.

[sfPlayer1]

The colon is extra noise, owner should be optional and discouraged for uid named members to avoid missing overridden methods, propagation should be disallowed per spec, classes have no owner, everything has to use uids, volatile is imo unreasonable at this point/out of scope.

Volatile is generally not sufficient to make something thread safe, it remains as racy as swapping an underlying collection with a thread safe one. All accesses need to treat the value as "unstable", which generally requires a larger edit. The above description about "weakening" is debatable, volatile incurs strong access ordering and doesn't make anything more extensible contrary to the other options.

The AT documentation should have a precise BNF grammar and specifically warn about the following:

• Removing final or private from classes or methods can lead to significantly reduced performance and memory overhead from virtual calls and vtables
• Removing final from fields can lead to thread safety problems. Final instance fields are guaranteed to be initialized and visibly set to other threads when construction finished, normal fields are not. This matters when exposing a new object to another thread without further synchronization.
• Removing final from static fields may also miss any compile time constant folding and incur a performance penalty from preventing runtime constant folding by the JIT compiler. JavaC turns accesses to primitive or String field accesses to inline or constant pool values, merges them with other values or elides them completely as appropriate. For the JIT compiler static final fields are the only constants, everything else is being treated as mutable at this time.
• Any change increases the incompatibility risk by exposing internals that may have been modified by other mods or may have been expected to work in a certain limited way
• Increasing the visibility of a method may change the resolution result with overloads or conflicting signatures

[Runemoro]

What about (optionally, depending on a setting in build.gradle) making everything public and having loom infer the minimum necessary access transformers? This would avoid having to recompile the code after every access transformer change, and it would also be very easy to use.

To make sure people are aware when they use private things, an annotation could be added to the things that are not actually public and warnings (or errors?) can be generated unless the method/class using them has an @AllowPrivateAccess annotation.

[liach]

Imo we should not touch private stuff, but handling/transforming package-private content to be protected/public with extra mixin hook or some sort of access transformer should be allowed.

[LemmaEOF]

yeah, classes are the primary issue here. There are juuust enough private classes in key areas to be a major hassle, so either A) transforming all private/package-private classes to be public or B) adding ATs to do that would be wonderful.

[modmuss50]

Is there any performance/possible issues with making them all public?

At runtime it could be baked into the intermediary jar.

[Frontrider]

I think at-s would be a good choice. Let me tell the game "this needs to be public" instead of trying to figure out what we need to make public, or just making everything public (bad idea).

It could also replace some of the existing apis, like the fabric block settings, as we can use the original (those should reduce maintainance effort on the side of fabric, even if just by a bit).

[liach]

blowing all classes to be public instead of package private should be relatively safe compared to making private methods protected. there is no potential conflict.

[liach]

package private fields/methods should not be transformed; mixin already has a good take on them.

[modmuss50]

This has now been handled with 5093f50

Only took 15 months.