Skip to content

Conversation

@jynus
Copy link
Contributor

@jynus jynus commented Jun 17, 2023

This tutorial had 2 main issues: the code wasn't adapted to Godot
4 syntax and API and the suggested tutorial contained information
to workaround the (at the time) non existence of the draw_arc()
function.

A general update has been made, keeping the general structure but
changing the example for something more useful and fun
(replicating the godot logo with draw primitives) and an
additional example with a dynamically built drawing (dynamic line).

Too many informalities have been removed on the initial paragraph.
Fixed references to Texture instead of Texture2D.
Fixed old style of exporting properties to the editor.
Fixed inconsistencies calling Node2D, myNode2D and customNode2D.
Moved examples to the end of the page, leaving general information
at the top, and completing that info with images and references.

Fixes #4184
Fixes #7521

@Piralein Piralein added bug area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.0 labels Jun 18, 2023
@jynus jynus marked this pull request as draft June 18, 2023 18:07
@jynus jynus force-pushed the drawing branch 2 times, most recently from c2419d9 to 7de26d9 Compare June 19, 2023 00:54
@jynus jynus changed the title [DO NOT MERGE YET] Update custom drawing in 2d tutorial to Godot4 Update custom drawing in 2d tutorial to Godot4 Jun 19, 2023
@jynus
Copy link
Contributor Author

jynus commented Jun 20, 2023

I have simplified the last part, I think it makes no sense to have complex code on a tutorial- I will send an amend soon after I test following it works.

@jynus jynus force-pushed the drawing branch 2 times, most recently from 76698e2 to 16693c0 Compare June 21, 2023 13:16
@jynus
Copy link
Contributor Author

jynus commented Jun 21, 2023

I think this version makes more sense- tutorials should be simple and show clear examples on how to use a library, even at the cost of showing more useful, and more realistic code.

@jynus jynus marked this pull request as ready for review June 21, 2023 13:23
@jynus
Copy link
Contributor Author

jynus commented Sep 1, 2023

Any update on this? I know it is a large change, but I think it is better than the old tutorial; and we can always improve on top of it- or if you can provide me some feedback I would appreciate it.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Style, syntax, and formatting details

@AThousandShips
Copy link
Member

Also please use rebasing to update your branch, see here

@jynus
Copy link
Contributor Author

jynus commented Sep 1, 2023

I think I applied all suggestions, but please give me some time to go over the tutorial again, to make sure I didn't miss anything and neither gdscript nor csharp code got broken while amending it, and can be followed well by a beginner.

@AThousandShips
Copy link
Member

You have missed a few in the middle

@jynus jynus force-pushed the drawing branch 2 times, most recently from 89e7c0c to 8881043 Compare September 5, 2023 17:54
@jynus
Copy link
Contributor Author

jynus commented Sep 5, 2023

I have fixed a few additional typos and minor expressions while redoing the tutorial for both gdscript and C#, also ensuring there was a lower possibility of confusion. While I cannot be sure 100% this is error-free, I feel now more confident after my last amend and this is ready for review.

@jynus
Copy link
Contributor Author

jynus commented Dec 20, 2023

Will now squash, rebase and do the pending changes (_t) before testing.

@jynus
Copy link
Contributor Author

jynus commented Dec 20, 2023

FYI for the mean time, I just merged #7994

While rebasing I noticed the merged quick fix has a bug. Not a big deal, as the rebase I did will fix it anyway. This is why it is important to test the code- which is my next task here.

@jynus
Copy link
Contributor Author

jynus commented Dec 20, 2023

Build failed after rebase due to cd92be0 , updating that too.

@jynus
Copy link
Contributor Author

jynus commented Dec 20, 2023

I am rechecking now all code snippets, please give me some time. I think it is important to go over it at least once.

@jynus jynus marked this pull request as draft December 20, 2023 13:58
@jynus jynus marked this pull request as ready for review December 20, 2023 15:20
@jynus
Copy link
Contributor Author

jynus commented Dec 20, 2023

I am happy with the current version, to my knowledge I have applied all suggestions and I have also tested that the functionality keeps working.

This tutorial had 2 main issues: the code wasn't adapted to Godot
4 syntax and API and the suggested tutorial contained information
to workaround the (at the time) non existence of the `draw_arc()`
function.

A general update has been made, keeping the general structure but
changing the example for something more useful and fun
(replicating the godot logo with draw primitives) and an
additional example with a dynamicaly built drawing (dynamic line).

Too many informalities have been removed on the initial paragraph.
Fixed references to Texture instead of Texture2D.
Fixed old style of exporting properties to the editor.
Fixed inconsistencies calling Node2D, myNode2D and customNode2D.
Moved examples to the end of the page, leaving general information
at the top, and completing that info with images and references.
Removed references to an old demo project (Tetris).

Fixes godotengine#4184
Fixes godotengine#7521

Co-authored-by: Raul Santos <raulsntos@gmail.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@AThousandShips AThousandShips removed the needs work Needs additional work by the original author, someone else or in another repo label Dec 20, 2023
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.

The C# code looks good and works as expected, thanks for contributing!

@skyace65 skyace65 merged commit 4a944b2 into godotengine:master Dec 22, 2023
@skyace65
Copy link
Contributor

Thanks! And thanks for dealing with the long review process, this is definitely a major improvement for the docs.

@jynus jynus deleted the drawing branch December 23, 2023 00:47
@jynus
Copy link
Contributor Author

jynus commented Dec 23, 2023

Thanks, I learned a lot, so hopefully future patches I can do them faster. Please if you have some linters you can recommend me for style checking, I would welcome them, specially on the C# side.

I would also like to thank you all for the help and the patience with me. :-)

mhilbrunner pushed a commit that referenced this pull request Jan 25, 2024
Update custom drawing in 2d tutorial to Godot4
@mhilbrunner
Copy link
Member

Cherry-picked to 4.2.

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

Labels

area:manual Issues and PRs related to the Manual/Tutorials section of the documentation bug cherrypick:4.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Outdated docs: Custom Drawing in 2D Edit Custom drawing in 2D

7 participants