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

DOCS: Improved script API docs of InputAction.CallbackContext #2064

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Dec 2, 2024

Description

Improved script API Doscs of InputAction.CallbackContext:

  • Added example
  • Extended description with links for struct type.

Testing status & QA

Reviewed generated XML docs in IDE inspector (Rider)
Generated docs via package docs tool.

Overall Product Risks

  • Complexity: very small
  • Halo Effect: very small

Comments to reviewers

Check language, compliance and example.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@ekcoh ekcoh requested review from duckets and removed request for duckets December 2, 2024 11:42
@ekcoh ekcoh marked this pull request as ready for review December 2, 2024 13:23
@ekcoh ekcoh requested a review from duckets December 2, 2024 13:23
@lyndon-unity lyndon-unity added the DocsQualityWeek2024 Temporary label for docs week label Dec 2, 2024
@ekcoh
Copy link
Collaborator Author

ekcoh commented Dec 2, 2024

I noticed this one has malformed xmldoc so need to revisit that

Copy link
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

Small correction needed in the example code

Copy link
Collaborator

@ritamerkl ritamerkl left a comment

Choose a reason for hiding this comment

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

LGTM, mainly minor comments

/// The callback context provides means to consume events (push-based input) as part of an update when using
/// input action callback notifications, e.g. <see cref="InputAction.started"/>,
/// <see cref="InputAction.performed"/>, <see cref="InputAction.canceled"/> rather than relying on
/// pull-based reading.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a piece of documentation to link to (push/pull based reading) to clarify this statement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just referencing the general concept of push vs pull architecture, i.e. callbacks vs polling in this case. Would it be better to switch to those?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just wondering if there is a documentation about the push/pull architecture, differences and advantages in the InputSystem, then this would be a good place to link it, but if there isn't it is fine I think.

/// {
/// // Note: Assumes the underlying value type is Vector2.
/// Vector2 value = context.ReadValue&lt;Vector2&gt;();
/// Debug.Log($"Value is: {value}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant line in my opinion add something up to a vector (eg the player position)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair, I originally had the read value inside the log call but it was more difficult to read. Also I am torn around what to do with the value since the code should compile and ideally do something useful if copied over into a script and entering playmode was my though. Not sure what the suggestion is? To have a public Vector3 direction so its visible in the Inspector?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That, or maybe modifiying the ridgid body of the gameobject directly?
eg ridgidBody.velocity = value*speed; (where ridgidbody is a public or serialized field that needs to be assigned in the editor and speed can be a private float field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would vote for the transform in this case otherwise, if the object has a mesh renderer, otherwise there is nothing to be seen. If rigidBody is used, then we need to add additional detail about that they must reconfigure the system to use FixedUpdate etc or sample value and use additional FixedUpdate to use sampled values. It seems @duckets was fine using Debug.Log at least

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, the only reason why I proposed using the ridgid body was that translating the transform directly is a rather bad practice in general using physics. So I am not sure about how to show case it without messing up with best practices from other areas. Maybe in that case a log would be the safest option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I am slow but why do we need to use physics?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I need to rephrase that: for a game using physics translating a player directly using the transform component is a bad idea, since it will not align with the physics of the game (the gravity, wind and other forces). That's why I am wondering if showing that in an example is a bad idea.

/// {
/// // ReadValueAsButton attempts to interpret the value as a button.
/// bool value = context.ReadValueAsButton();
/// Debug.Log($"Button state is: {value}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above comment. Is it usual to use prints in the docs examples?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly I am not sure what is recommended, other suggestions to do something useful that indicates that it works if copied into a .cs file without adding much more code to the sample?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be enough even to call an empty function like if(value){Jump();} -> Jump() { //insert player jump logic here OR Debug.Log("Jump"); } for example

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, I think it's complaining on a high level and is okay to handle like that, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think about it an update once CI / package tool issues are resolved

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

Successfully merging this pull request may close these issues.

4 participants