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

Port code examples to C# of classes beginning with A and B #40978

Merged
merged 1 commit into from
Sep 26, 2020

Conversation

HaSa1002
Copy link
Contributor

@HaSa1002 HaSa1002 commented Aug 2, 2020

First PR in my code example port effort.
See #40613 for more details.
Since there are a lot classes (and a lot less having examples) I group the changes together, but I don't really know what is practical so ¯\(ツ)

I created this code translation tool to speed up the conversion process.

I left the sort as it is, because I don't know how and if it is done in C#

@HaSa1002 HaSa1002 requested a review from a team as a code owner August 2, 2020 18:19
doc/classes/ArrayMesh.xml Outdated Show resolved Hide resolved
@Calinou Calinou added this to the 4.0 milestone Aug 2, 2020
@HaSa1002 HaSa1002 changed the title Convert code examples of classes beginning with A and B Port code examples to C# of classes beginning with A and B Aug 3, 2020
@HaSa1002
Copy link
Contributor Author

HaSa1002 commented Aug 3, 2020

Fixed issue in Animation.xml

Does anybody have an idea, how to parse $Node["some/property"]using static analysis? I might use the $ as strong indication and/or if there is a slash in the subscription operator. Anyhow I updated the codetranslator again and made it more reliable as well as adding some features

doc/classes/Button.xml Outdated Show resolved Hide resolved
doc/classes/AESContext.xml Outdated Show resolved Hide resolved
doc/classes/AStar.xml Outdated Show resolved Hide resolved
doc/classes/Animation.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/bool.xml Outdated Show resolved Hide resolved
@HaSa1002 HaSa1002 force-pushed the docs-code-1 branch 2 times, most recently from 7a00675 to 7f7520e Compare September 4, 2020 10:54
@HaSa1002 HaSa1002 force-pushed the docs-code-1 branch 2 times, most recently from d7b10fb to 2d5cac6 Compare September 4, 2020 12:02
doc/classes/Array.xml Outdated Show resolved Hide resolved
@HaSa1002
Copy link
Contributor Author

HaSa1002 commented Sep 4, 2020

fixed a ton of codeblock which should have been codeblocks
fixed suggestions and reviewed the code again.

@HaSa1002 HaSa1002 force-pushed the docs-code-1 branch 2 times, most recently from f2d8e2e to 68cc31a Compare September 4, 2020 13:13
doc/classes/AcceptDialog.xml Outdated Show resolved Hide resolved
doc/classes/Animation.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/ArrayMesh.xml Outdated Show resolved Hide resolved
doc/classes/ArrayMesh.xml Outdated Show resolved Hide resolved
doc/classes/ArrayMesh.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Button.xml Outdated Show resolved Hide resolved
doc/classes/bool.xml Show resolved Hide resolved
@aaronfranke
Copy link
Member

@HaSa1002 Since #42000 was merged, you can add array concatentation. I suggest adding these lines:

// Array concatenation is not possible with C# arrays, but is with Godot.Collections.Array.
var array1 = new Godot.Collections.Array("One", 2);
var array2 = new Godot.Collections.Array(3, "Four");
GD.Print(array1 + array2); // Prints [One, 2, 3, Four]

@HaSa1002
Copy link
Contributor Author

@aaronfranke Added the array concatentation example and did another review pass. All code examples should compile. I removed the wrong example for Array.sort Godot.Collections.Array has no support for it.

@@ -6,10 +6,16 @@
<description>
Allows control of [AnimationTree] state machines created with [AnimationNodeStateMachine]. Retrieve with [code]$AnimationTree.get("parameters/playback")[/code].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe remove the "Retrieve with .." line, since the codeblock shows how to get it. (Or just remove the code tag)

Comment on lines -9 to +18
[codeblock]
[codeblocks]
[gdscript]
var state_machine = $AnimationTree.get("parameters/playback")
state_machine.travel("some_state")
[/codeblock]
[/gdscript]
[csharp]
var stateMachine = GetNode&lt;AnimationTree&gt;("AnimationTree").Get("parameters/playback") as AnimationNodeStateMachinePlayback;
stateMachine.Travel("some_state");
[/csharp]
[/codeblocks]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What has this example to do with the class?

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Looks really good, found one more problem though.

astar.AddPoint(1, new Vector3(0, 0, 0));
astar.AddPoint(2, new Vector3(0, 5, 0));
astar.ConnectPoints(1, 2);
Vector3 res = astar.getClosestPositionInSegment(new Vector3(3, 3, 0)); // Returns (0, 3, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Vector3 res = astar.getClosestPositionInSegment(new Vector3(3, 3, 0)); // Returns (0, 3, 0)
Vector3 res = astar.GetClosestPositionInSegment(new Vector3(3, 3, 0)); // Returns (0, 3, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@HaSa1002 HaSa1002 force-pushed the docs-code-1 branch 2 times, most recently from d331fef to 37a3cf4 Compare September 25, 2020 13:03
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

This should be good now. The CI failure is unrelated to this PR.

@akien-mga
Copy link
Member

It would need a last rebase so that we can get CI to pass (the error is on the master commit this PR is currently based on).

@HaSa1002
Copy link
Contributor Author

rebased

Only existing GDScript code examples are converted and added to the
docs.
This is the first batch include classes beginning with A and B.

Included classes:
 * AcceptDialog
 * AESContext
 * Animation
 * AnimationNodeStateMachine
 * AnimationNodeStateMachinePlayback
 * AnimationNodeStateMachineTransition
 * Array
 * ArrayMesh
 * AStar
 * AStar2D
 * Bool
 * Button
@akien-mga akien-mga merged commit 84cec77 into godotengine:master Sep 26, 2020
@akien-mga
Copy link
Member

Thanks a ton!

Comment on lines +36 to +37
var array1 = new Godot.Collections.Array("One", 2);
var array2 = new Godot.Collections.Array(3, "Four");
Copy link
Member

Choose a reason for hiding this comment

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

Neikeq pointed out that we can also use { and }, which may be preferred...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I switched to that syntax in later PRs

var array = new Godot.Collections.Array{2, 4, 6, 8};
if (array.Contains(2))
{
GD.Print("Containes!");
Copy link
Member

Choose a reason for hiding this comment

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

Typo, Containes -> Contains

HaSa1002 added a commit to HaSa1002/godot that referenced this pull request Jun 11, 2021
Includes:
 * Variant
 * Viewport

and two fixes in Array that were pointed out in godotengine#40978
VisualScript classes are skipped on purpose.
That is the final commit of the inital code porting to C#. :)
This pull request was closed.
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.

7 participants