Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify that ModifyArgs.index is the absolute index, not relative #666

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sollace
Copy link

@Sollace Sollace commented May 19, 2024

If the target method has the argument (double a, double b, double c, float d, float e) an index of 1 targets the double b argument, not float e as one might assume from the second paragraph.

If the target method has the argument `(double a, double b, double c, float d, float e)` an index of 1 targets the `double b` argument, not `float e` as one might assume from the second paragraph.
@Geolykt
Copy link

Geolykt commented May 19, 2024

What exactly do you mean with relative here? Because I have the small feeling that using the word "absolute" without elaborating on it might be a bit confusing to some people.

@Sollace
Copy link
Author

Sollace commented May 20, 2024

What exactly do you mean with relative here? Because I have the small feeling that using the word "absolute" without elaborating on it might be a bit confusing to some people.

As the commit description says:

If the target method has the argument `(double a, double b, double c, float d, float e)`
an index of 1 targets the `double b` argument, not `float e` as one might assume from the second paragraph.

Relative means the index starts with the first matching argument (same type).
Absolute is index starts with the first argument.

If you look at the original doc:

 * <p>Gets the argument index on the target to set. It is not necessary to
     * set this value if there is only one argument of the modifier type in the
     * hooked method's signature. For example if the target method accepts a
     * boolean, an integer and a String, and the modifier method accepts and
     * returns an integer, then the integer parameter will be automatically
     * selected.</p>

This description doesn't make that clear, and if you read the second sentence you get the impression that the index is counted separately for each type (relative) which makes it extremely confusing when an inject like:

@ModifyArg(method = "attack", at = @At(value = "INVOKE", target = "setVelocity(DDDFF)"), index = 1)
private float modifyDivergence(float divergence) {

- doesn't work because from reading the docs you're led to expect 1 to point to the second float parameter, not the second double.

@Sollace
Copy link
Author

Sollace commented May 20, 2024

I got the "absolute" vs "relative" descriptor from asking on the Fabric discord why the above injector doesn't work, and the answer I got was "it should be an absolute index".

@Geolykt
Copy link

Geolykt commented May 20, 2024

While I understand the semantics of index and thus can infer your definition of absolute based on that knowledge, the problem I have is that someone that is unfamiliar with what "absolute" means will get tripped up and confused after reading that word and attribute it a meaning it doesn't have (e.g. them thinking that an index = 0 modifies the reciever, which isn't the case I think? I might have confused myself right now lol. Another possible pitfall that could stem from excessive thought is attributing types of computational type category 2 to be two indices long). The other scenario is that they completely ignore that word in which case it doesn't help anything. But perhaps you (or someone else) are the better judge of that.

@Sollace
Copy link
Author

Sollace commented May 20, 2024

I am open to a better way to describe it. IMO I still think it's better to have it than to not.

@Earthcomputer
Copy link

I don't understand how you could possibly get confused with the existing documentation. Why on Earth would you assume that the index is relative? Generally indexes are absolute unless otherwise specified.

@Earthcomputer
Copy link

I think there is a very simple typo in the original documentation. on -> of. Then any possible confusion is cleared.

@Mumfrey
Copy link
Member

Mumfrey commented May 20, 2024

@Sollace wrote:
Relative means the index starts with the first matching argument (same type).
Absolute is index starts with the first argument.

Except that Mixin is already consistent in its use of ordinal to descibe "index within a group" and index to mean "index within scope". Personally I think the language of the document is fine and if anything it's just the example which could be tweaked to include a disambiguating circumstance, such as adding an differently-typed argument at the start of the example candidate method which shows that it is included in the consideration for index.

@Earthcomputer wrote
I think there is a very simple typo in the original documentation. on -> of. Then any possible confusion is cleared.

Sure, I'll buy that for a dollar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants