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

(CommandHelper) Non-static console commands throws generic error #123

Closed
8BitShadow opened this issue Mar 23, 2020 · 5 comments · Fixed by #124
Closed

(CommandHelper) Non-static console commands throws generic error #123

8BitShadow opened this issue Mar 23, 2020 · 5 comments · Fixed by #124
Labels
enhancement New feature or expansion of a feature

Comments

@8BitShadow
Copy link

8BitShadow commented Mar 23, 2020

If you forget to set a CC method to static then you'll get a generic "method arguments are incompatible" error during startup, regardless of what's written inside the method.

Example:
[ConCommand(commandName = "test", flags = ConVarFlags.None, helpText = "test")]
public void CCTest(ConCommandArgs args){ }
Instead of:
[ConCommand(commandName = "test", flags = ConVarFlags.None, helpText = "test")]
public static void CCTest(ConCommandArgs args){ }

I can't get the exact point that causes the error as DNSpy wont go that far, but the error occurs somewhere directly after the while loop of R2API.Utils.HandleCommandsConvars() - outside of it (step next three times from the end of HandleCommandsConvars(), looks like it's @this.concommandCatalog[conCommandAttribute.commandName.ToLower(CultureInfo.InvariantCulture)] = new RoR2.Console.ConCommand {... that's doing it).

CommandHelper should check if the method being registered is static or not and skip it if it isn't, instead of throwing a generic error, also (if possible) warning the user which CCs have been skipped, so that the user can easily tell what the problem is and the mod author can easily fix it.

@harbingerofme
Copy link
Collaborator

I think we already do this for convars, I'll add the checks for the cc's later today

@harbingerofme
Copy link
Collaborator

@wildbook might be interested in this as well.

@xiaoxiao921
Copy link
Member

xiaoxiao921 commented Mar 23, 2020

iirc the static is required because every commands the game uses also has it. And that wouldn't make sense to have the command method be dependant of an object instance to be honest. Though it's true that the error you get isnt really that descriptive

@8BitShadow
Copy link
Author

The point is not that it shouldn't require static, but rather that there needs to be a catch/check at some point to stop it from throwing a generic message when trying to register a non-static CC and instead throw a message saying something like "{x} CC failed to register as it is not static".

It's not exactly an API breaking thing, but it does save people a lot of time "chasing ghosts".

@xiaoxiao921
Copy link
Member

My point was that the way commands are registered is that the MethodInfo (see the signature of the command method here) must be static.
We simply reused the same way the game did for gathering conCommands https://github.com/risk-of-thunder/R2API/blob/master/R2API/Utils/CommandHelper.cs#L96
You got a point like i said in my message earlier that the error isnt really descriptive, we'll fix it

@harbingerofme harbingerofme added the enhancement New feature or expansion of a feature label Mar 23, 2020
tristanmcpherson added a commit that referenced this issue Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or expansion of a feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants