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

Add C# examples to PropertyTweener docs #99736

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

Conversation

Bossdell113
Copy link

@Bossdell113 Bossdell113 commented Nov 27, 2024

This is something that I am doing for a school project. I saw an issue posted to the Godot-docs repo about missing C# examples in the PropertyTweener class reference page. It is labeled as issue #9924 on the docs repo, not the main one, and can be found easily in the good first issues list.

I have added C# examples for the following methods.

as_relative
from
from_current
set_custom_interpolator

I used tools like the code translator as well as cross referencing with other areas of the documentation, primarily the Tween class reference was really helpful, but there are still some parts of this that I am not completely sure about. I doubt that I am getting this perfect on my first try here, this is the first time I have done anything like this, so feedback for my code and tips for how I can improve my examples and make them more accurate would be greatly appreciated. I hope I can take any feedback/criticism and use it to make this something that is up to standard.

Bugsquad edit: closes godotengine/godot-docs#9924

doc/classes/PropertyTweener.xml Outdated Show resolved Hide resolved
doc/classes/PropertyTweener.xml Outdated Show resolved Hide resolved
doc/classes/PropertyTweener.xml Outdated Show resolved Hide resolved
doc/classes/PropertyTweener.xml Outdated Show resolved Hide resolved
{
Tween tween = CreateTween();
// Interpolate the value using a custom curve.
Tween.TweenProperty(this, "position:x", 300.0f, 1.0f).AsRelative().SetCustomInterpolator(TweenCurve);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Tween.TweenProperty(this, "position:x", 300.0f, 1.0f).AsRelative().SetCustomInterpolator(TweenCurve);
Callable tweenCurveCallable = Callable.From((float value) => TweenCurve(value));
tween.TweenProperty(this, "position:x", 300.0f, 1.0f).AsRelative().SetCustomInterpolator(tweenCurveCallable);

This one compiled and ran for me, though I'm not confident this is the preferred way to create a callable in this case. Needs a review from a C# expert.

(Used this page: C# Callable)

@tetrapod00 tetrapod00 requested a review from a team November 27, 2024 02:53
@tetrapod00 tetrapod00 changed the title Adding C# examples to PropertyTweener.xml Add C# examples to PropertyTweener.xml Nov 27, 2024
@tetrapod00 tetrapod00 changed the title Add C# examples to PropertyTweener.xml Add C# examples to PropertyTweener docs Nov 27, 2024
@tetrapod00 tetrapod00 added this to the 4.x milestone Nov 27, 2024
@Bossdell113
Copy link
Author

I have amended this PR with the changes that were suggested. I don't know if I was supposed to create a new commit in the process but there is a new one here with the changes to the code.

@tetrapod00
Copy link
Contributor

Yeah, I'm not sure exactly what happened but it looks like you have two commits instead of a single amended commit. It's not a big deal, just means that you'll have to squash after all the reviews.

I'm marking all the changes that I suggested and that you applied as "resolved" (except for the callable one). In the future, if someone suggests a change and you apply it exactly as-is, feel free to mark it resolved yourself (but leave it unresolved if you're unsure, or if you didn't apply it exactly as suggested).

Now we're just waiting on a C#-specific review; and once that's done you can rebase/squash into one commit. Good work!

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

The C# code examples look good to me, and I tested them in-editor. But this still needs a review from someone more familiar with C# callables and with C#-specific code style for examples.

@Bossdell113

This comment was marked as resolved.

@tetrapod00

This comment was marked as resolved.

@Bossdell113

This comment was marked as resolved.

@tetrapod00

This comment was marked as resolved.

@Bossdell113

This comment was marked as resolved.

@Bossdell113

This comment was marked as resolved.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to the C# documentation.

doc/classes/PropertyTweener.xml Outdated Show resolved Hide resolved
doc/classes/PropertyTweener.xml Outdated Show resolved Hide resolved
doc/classes/PropertyTweener.xml Outdated Show resolved Hide resolved
@Bossdell113
Copy link
Author

Ok, i'll add these changes 👍

@Bossdell113
Copy link
Author

I have made the suggested changes to set_custom_interpolator

@Bossdell113
Copy link
Author

So all the checks passed, what next?

@tetrapod00
Copy link
Contributor

So all the checks passed, what next?

"All checks have passed" in the UI just means that the automated CI checks have passed. It's a prerequisite for approving a PR but not the only thing that needs to be done. You're still waiting for @raulsntos to come back and approve your changes. Once that happens, you'll have an approval from a docs reviewer (me) and one from a C# reviewer (raulsntos), and with two approvals from the relevant parties, the PR will be fully approved. Some time after that, but not immediately, it will be merged.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@Bossdell113
Copy link
Author

Should I squish the commits then?

@tetrapod00
Copy link
Contributor

Yes, please squash! Forgot to mention that in the summary of next steps

@Bossdell113
Copy link
Author

Ok I think I did it right

@tetrapod00
Copy link
Contributor

Looks like you did. I'm not sure if it's a hard requirement for merging, but it would also be nice if you amended the commit message to be more descriptive, like the PR title currently is (I changed the PR title for you earlier).

Adding C# examples to PropertyTweener Docs for the following methods

as_relative
from
from_current
set_custom_interpolator

Co-Authored-By: tetrapod <[email protected]>
@Bossdell113
Copy link
Author

So I think I amended it correctly but I don't see the changed description.

@Bossdell113
Copy link
Author

Wait nevermind I see it

@tetrapod00
Copy link
Contributor

image

It's showing up like this for me. Ideally it would be something more like:

Add C# examples to PropertyTweener Docs

Docs for the following methods:

as_relative
from
from_current
set_custom_interpolator

instead of the current:

 Update PropertyTweener.xml

Adding C# examples to PropertyTweener Docs for the following methods

as_relative
from
from_current
set_custom_interpolator

If you can't figure this out, it's probably fine, since the PR title is already good and the commit title and description are already reasonably descriptive 🤷. If it's actually a problem then the production team will let you know before merging.

@Bossdell113
Copy link
Author

Are the other checks supposed to happen again?

@tetrapod00
Copy link
Contributor

The checks aren't automatically run, since you're a first-time contributor. I thought the last change you made didn't change the content, so they should still pass. But it doesn't hurt to run the CI again.

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

Successfully merging this pull request may close these issues.

Missing C# examples for PropertyTweener class
3 participants