Use Access Transformers instead of reflection

[Caellian]

How good would that be from automation perspective? I mean, weren't there problems with gradle and Access Transformers not working properly with IntelliJ or something?

On 2 August 2015 at 16:25, Calclavia notifications@github.com wrote:

— Reply to this email directly or view it on GitHub https://github.com/NOVA-Team/NOVA-Core/issues/163.

[RX14]

The only reason to use Ats over reflection is performance, I say use reflection because Ats are a pain in the f*\ ass.

[Caellian]

Yeah, that's what I was thinking. If you don't do reflection in the loop it really isn't that much of a problem and accesing an object takes probably less than a milisecond. So even if you had a lot of reflection going, it would only add about 1 second to the loading time. I would rather wait 1 second than torture someone like this.

On 2 August 2015 at 22:10, Chris Hobbs notifications@github.com wrote:

The only reason to use Ats over reflection is performance, I say use reflection because Ats are a pain in the f*\ ass.

— Reply to this email directly or view it on GitHub https://github.com/NOVA-Team/NOVA-Core/issues/163#issuecomment-127065848 .

[Kubuxu]

Reflection holds minimal (almost non existent) performance penalty under one condition. You are not performing inspection (finding fields, methods) in the loop.

[RX14]

yeah, if you store the method reference, isn't it quite cheap to call?

[Kubuxu]

It is.

[Caellian]

How bad would be checking all objects in a tree if they are annotated every game tick using reflection? Very bad then I guess, I should probably change my physics engine. LOL

On 2 August 2015 at 22:39, Chris Hobbs notifications@github.com wrote:

yeah, if you store the method reference, isn't it quite cheap to call?

— Reply to this email directly or view it on GitHub https://github.com/NOVA-Team/NOVA-Core/issues/163#issuecomment-127068559 .

[Kubuxu]

You should cache field instances.

[Victorious3]

ATs are easy to use, run once, done. It's not because of the performance, it is because a normal call is a bit easier to write than

try { 
ReflectionHelper.findMethod(class, instance, new String[] { "func_XXX", "method" }, String.class, int.class).invoke(astring, anint) } 
catch (Exception e) {}

See why they are useful?

[RX14]

But they are a pain in the ass because you have to copy the ATs of all the dependencies into src/api/resources then rerun setupDecompWorkspace and don't accidentally run gradle clean or you have to do it all over again. It also increases the build time for CI significantly.

[Victorious3]

@RX14 Last time I checked it was only the wrapper. I don't know if you want to build 1.7.10 along with 1.8, but that shouldn't happen in the first place.

[RX14]

@Victorious3 the issue happens when you define a maven dependnecy on something deobfuscated which needs an AT. We don't do that currently but it might be needed in the future.

[Victorious3]

@RX14 Nope, derived ATs are runtime.

[RX14]

@Victorious3 but the AT isn't used when recompiling minecraft using setupDecompWorkspace, so the IDE can't compile because the AT isn't there. It is an issue, you just havent come across it.

[Victorious3]

@RX14 Then write a script for it, shouldn't be that hard... Really, ATs are 100 times nicer to use than reflection, and I wouldn't just do without because of something on the build side. Especially because it is not used by anything other than the wrapper.

[RX14]

On the other hand, we already have ATs, so adding more instead of reflection is better. Either use ATs or reflection, not both.

[Victorious3]

For a library like this I would definitely go for ATs, reflection is useful if you only do it a few times, but on a larger scale an AT is worth it.

[Caellian]

@RX14 Make a plugin. Hehe https://docs.gradle.org/current/userguide/custom_plugins.html

[RX14]

For a library, I would use reflection, because of the gradle issue. The wrapper however is not a library, so Ats are fine I suppose.

[RX14]

@Caellian If it could be done from a plugin easily, @AbrarSyed would already have done it, IIRC it was scheduled for ForgeGradle 2 because of a change in behaviour that made it possible.

[Victorious3]

@RX14 Throw all derived ATs together, create a new file, done? Well, doesn't sound nice at all, but if it comes down to it...

[RX14]

@Victorious3 It's to do with configuration immutability, and resolve orders. Internal gradle stuff that makes it difficult to do X before Y.