Skip to content
New issue

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

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

Already on GitHub? # to your account

[REG-1255] RGAction attribute docs #31

Merged
merged 4 commits into from
Sep 11, 2023
Merged

[REG-1255] RGAction attribute docs #31

merged 4 commits into from
Sep 11, 2023

Conversation

abeizer
Copy link
Collaborator

@abeizer abeizer commented Sep 11, 2023

  • Changed the "RGAction" section into one on defining actions using the "RGAction" attribute
  • Organized images into a subfolder to reduce visual clutter


If your bot code attempts to call an action whose script is not assigned to its GameObject,
then the Regression Games SDK will produce an error and terminate that bot.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an assumption, I've never actually tried this. If someone can tell me what actually happens I'll update this doc. If not, I'll try this out myself and see

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless robb changed it, that isn't what used to happen. In the past it would just ignore that action.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I didn't touch that error handling. The only new error handling introduced would be assigning an incorrect RGAction script to a game object, because the RGAction calls a 'GetComponent' to find the expected monobehavior.

Copy link
Contributor

@vontell vontell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small comments!

#### Arguments
None
By default, the action's name matches the method's, but this can be overridden by passing a different name to `RGAction`.
Any parameters that the method accepts will be valid arguments in your bot code. Their names and data types must match.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to clarify what "types must match" here - I believe this means that the arguments being passed from the bot code must match the primitive or serialized JSON types of the arguments?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree we need clarity here. As I understand it. The arguments from your bot, must match the argument names and types in the C# code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - updated for clarity

The name of this action (i.e. an identifier).
:::info
We recommend using primitive data types for `RGAction` parameters.
Full support for reference types is coming soon.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like instead of "reference" types, maybe just non-primitive?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question here... are array types currently supported ?

Copy link
Collaborator Author

@abeizer abeizer Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated phrasing.

I believe arrays should be supported, my understanding is that non-primitives become an issue when they contains a field of another non-primitive type. We're recommending "please don't use them" to simplify. @rcornwall-reg is this correct? If so, we could outline this more accurately in the docs, but I'll probably need some help with wording

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, here it is. I left a comment elsewhere but I tested it, and array types aren't supported. The serializer generates incorrect code:

return JsonUtility.FromJson<int[]>(paramJson);

We should put that as a new task 👍

// This is called using the name "OpenContainer"

[RGAction]
public void OpenContainer(bool isLocked)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be good to add an example (maybe in a different section) of adding a "proxy method" that uses primitive types for an action that uses non-primitive types.

i.e. They have a function called AttackPlayer(GameObject player), and we add a new one called:

[RGAction("AttackPlayer")
public void AttackPlayerProxy(int playerId)
{
    GameObject player = RGFindUtils....(playerId)
    AttackPlayer(player)
}

Basically, what I want them to know is that even if we don't support complicated types yet, there are ways around it, and it would be good for them to see an example of that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion, added


:::info
We recommend using primitive data types for `RGAction` parameters.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the question anymore, but I remember seeing something about array support. I tried it out and it does not work. The Serializer class generates

return JsonUtility.FromJson<int[]>(paramJson);

which gives a compiler error. We can put that as a new task 👍

Copy link
Contributor

@nAmKcAz nAmKcAz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving assuming aaron's feedback is addressed

@vontell vontell self-requested a review September 11, 2023 18:05
@vontell
Copy link
Contributor

vontell commented Sep 11, 2023

LGTM!

@vontell
Copy link
Contributor

vontell commented Sep 11, 2023

I believe this can be merged?

@abeizer
Copy link
Collaborator Author

abeizer commented Sep 11, 2023

I believe this can be merged?

Not yet, I have a couple questions I wanted to ask Robb but didn't get to hop into a call with him today.

@abeizer abeizer merged commit 5dfdd02 into main Sep 11, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants