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

Update 2D transform tutorial #6277

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Oct 7, 2022

Update "Viewport and canvas transforms" tutorial

  • incldue missing Window-transform
  • Update graphic to make the distinction between coordinate systems
    and transforms mode clear
  • Restructure "transform functions" section
  • Update functions to new Godot 4 conventions

Create a more detailed page in engine documentation about 2d coordinate
systems and 2d transforms.

changes this page: https://docs.godotengine.org/en/latest/tutorials/2d/2d_transforms.html
preview: https://github.com/Sauermann/godot-docs/blob/fix-transform-2d-update/tutorials/2d/2d_transforms.rst
new page preview: https://github.com/Sauermann/godot-docs/blob/fix-transform-2d-update/contributing/development/core_and_modules/2d_coordinate_systems.rst

Incorporates the changes from the recently merged godotengine/godot#59682, godotengine/godot#66688, godotengine/godot#66692 and godotengine/godot#68627.

@Calinou Calinou added the bug label Oct 7, 2022
@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Oct 7, 2022

Adding OP's documentation link reference https://docs.godotengine.org/en/latest/tutorials/2d/2d_transforms.html

@Sauermann
Copy link
Contributor Author

@TheYellowArchitect thank you for letting me know, that there is a functionality for previewing the page in its changed form. Added a link in the OP.

@TheYellowArchitect
Copy link
Contributor

TheYellowArchitect commented Oct 7, 2022

@Sauermann to explain the functionality preview change: Markdown renders differently (e.g. :ref:Node3D <class_Node3D> looks very different than the text format here on github, with a huge REF in front of the class name while rendered) so at least my workflow to review any docs request is to see the diff, then open the existing documentation page to confirm

Anyway, providing the existing docs page is a good reference for any user to understand what is different because the diff file of github highlights only the differences, and context is often needed which doesn't appear on diff file by default. Not important in this case, but had to post nonetheless as an excuse since I will soon dive into viewports myself hahaha

I suggest editing the OP link with .html (I apologize for my first post that I posted .rst, I meant to post the .html which I have now edited 😅 )

@Sauermann Sauermann force-pushed the fix-transform-2d-update branch from 2a20209 to 4a35902 Compare October 8, 2022 09:36
@TheYellowArchitect
Copy link
Contributor

That moment when a docs page adds many new sections instead of fixing bugs and typos, impressive 🎉

@Sauermann Sauermann force-pushed the fix-transform-2d-update branch 4 times, most recently from 3c923ab to 7208253 Compare October 22, 2022 10:26
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch 2 times, most recently from 2dac813 to 213601d Compare November 5, 2022 10:21
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch 3 times, most recently from 33954c5 to 7ccf1ec Compare November 19, 2022 16:09
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch from 7ccf1ec to 87f1be8 Compare November 26, 2022 20:31
@skyace65 skyace65 added the area:manual Issues and PRs related to the Manual/Tutorials section of the documentation label Jan 13, 2023
tutorials/2d/2d_transforms.rst Outdated Show resolved Hide resolved
tutorials/2d/2d_transforms.rst Outdated Show resolved Hide resolved
tutorials/2d/2d_transforms.rst Outdated Show resolved Hide resolved
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch from 87f1be8 to ea5c572 Compare January 17, 2023 23:11
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch 3 times, most recently from 6eebda0 to 5722b8d Compare January 27, 2023 21:20
@Sauermann Sauermann marked this pull request as ready for review January 27, 2023 21:24
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch from 5722b8d to 5e89720 Compare January 29, 2023 19:17
Sauermann referenced this pull request Feb 10, 2023
* Update some C# examples

- Rename members that have been renamed in Godot's C# API for 4.0.
- Change `delta` parameter type to `double`.
- Ensure parameters match base declaration.
- Other minor code fixes.

---------

Co-authored-by: Paul Joannon <[email protected]>
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch 2 times, most recently from 50d4c96 to 40e1045 Compare February 15, 2023 14:22
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch from 40e1045 to 34196a5 Compare February 26, 2023 14:36
@mhilbrunner
Copy link
Member

mhilbrunner commented Feb 27, 2023

@Sauermann One concern I have is that it replaces what currently is a very beginner-friendly docs page that answers basic questions with something very much more aimed at experts and that is more akin to something of detailed internal documentation of how everything works.

Both are useful. And we did decide to move more to a technical manual than beginner tutorial for the official docs, it seems.
Still, I worry that replacing the old with the new will just make every non-expert mentally check out after seeing that new graphic. I know I did :) To compare:

Old

old

New

new

And a lot of that may be because of the increased complexity in Godot 4.0 with all its window handling, sure. The new page certainly contains a ton more useful information. It just seems way less accessible.

Maybe it would make sense to reorder the page a bit, to move the graphic down? Not sure.

Maybe some input from @NathanLovato could help. Open for any input on this. My personal first impression is it may make sense to move the beginner-facing information up, the more complex details down on the page (or even split it into 2), but if y'all agree this is mostly interesting to experts anyway, it could also stay as is.

(Also: sorry this has been sitting here for a while :))

@Sauermann
Copy link
Contributor Author

Sauermann commented Feb 27, 2023

Thanks for the review. It is an interesting idea to merge the previous beginner-friendly (but slightly wrong) explanation/introduction with the details of this PR. I am also open for ideas on how to improve this page.

@NathanLovato
Copy link
Contributor

NathanLovato commented Feb 27, 2023

I'll give you a first answer, reviewing the general approach of the article, because we still have work for the 4.0 release.

The original article promises to discuss low-level details of how transforms work in the engine, so the amount of detail seems in line with this goal. But considering this is in the user manual, rather than engine documentation, I would personally either change the target to giving users just what matters in their work or move the page elsewhere.

I would move low-level details that are mostly required when contributing to the engine (i.e. knowledge that's not directly applicable when creating games in the editor) to the engine documentation.

Teaching-wise, the main thing I'm missing from the previous version and this PR are concrete takeaways or use cases for the reader in the intro. Something like:

You will learn how ... You will be able to use that to do [insert concrete thing here] and [insert other example here].

And you want to make sure the following lecture reaches those goals.

A teaching goal could be, for example, "gaining the required knowledge to code a custom 2D camera", if that's ever wanted.

The way I would approach teaching transforms in the user manual would be focusing a lot more on the fact that transform values pack position, rotation, and scale information, and how to read and manipulate those concretely, rather than covering the whole chain of transforms the engine applies to place and render images. And I'd cover the latter in engine documentation.


The image linked by @mhilbrunner is difficult to read. It tries to pack too much information, and an issue is that it has a lot of English text which won't be translatable or accessible to text-to-speech users. Images in the docs should generally not use more text than node types or symbols for this reason.

And again, it feels more like technical documentation for an engine contributor than what a user of the Godot editor would use in their projects.

In summary, my general feedback on this is that I don't think the goals and angle taken by the original article are the best fit for end user documentation. I feel that in its current state, it'd be a better fit for engine documentation.

@Sauermann
Copy link
Contributor Author

Sauermann commented Feb 27, 2023

Thanks for the review.
This rewrite is based on my experience in the issue tracker, that users often have problems with utilizing the correct coordinate system. Here are some examples:
godotengine/godot#28888 (comment)
godotengine/godot#29614 (comment)
godotengine/godot#59461 (comment)
godotengine/godot#59754 (comment)
godotengine/godot#61263 (comment)
godotengine/godot#70833 (comment)
godotengine/godot#72970 (comment)
godotengine/godot#85931 (comment)

So my aim was to provide a tutorial to help these users and as it turned out, this made it necessary to explain low-level-details. Perhaps we could start with these issue-reports in order to identify use-cases that are worth describing/focusing on in a tutorial.

@Sauermann
Copy link
Contributor Author

Sauermann commented May 7, 2023

I understand that there are some reservations against this PR in the end user documentation because of its complexity.

I feel that in its current state, it'd be a better fit for engine documentation

Could I get some guidance about how to use the collected information and the graphic as engine documentation?

@mhilbrunner
Copy link
Member

mhilbrunner commented May 7, 2023

@Sauermann Need to discuss this with the docs team, but currently, we got the Contributing > Engine Development > Godot file formats section, which has stuff like this: https://docs.godotengine.org/en/stable/contributing/development/file_formats/gdscript_grammar.html

It basically currently only has that and the TSCN format description, thats all. I could see renaming that "Godot File Formats" section to something like "Godot Engine Internals" and move this information there. Would be a good place to move a few other more internal/core development oriented information too.

I also wonder if we should move that section one level higher. 🤔 But IMO that would be a good place, I don't think we currently have a place for such info really, and it may be good to have.

@mhilbrunner
Copy link
Member

mhilbrunner commented May 7, 2023

After a brief discussion: https://docs.godotengine.org/en/stable/contributing/development/core_and_modules/index.html#getting-started-with-godot-s-source-code already has subpages for Core types, Object and Variant, so a page there for advanced Transform information would be a good fit.

So I guess to get this merged, we need:

  1. To split it between the information important for the average beginner and Godot gamedev and more advanced/detailed info that goes into a page in https://docs.godotengine.org/en/stable/contributing/development/core_and_modules/index.html#getting-started-with-godot-s-source-code
  2. To make another pass at making it a bit more accessible, as its a bit dense currently

@mhilbrunner mhilbrunner added the needs work Needs additional work by the original author, someone else or in another repo. label May 18, 2023
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch from 34196a5 to 71bdbe2 Compare July 10, 2023 21:25
@Sauermann
Copy link
Contributor Author

I have updated the PR to split the details into a engine documentation page and leave most of the 2d-transform-tutorial intact with the exceptions of some updates.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Looks great. This in an amazing piece of work.

My only other suggestion is to always use the full 'OS Window Manager' term, never leaving the "OS" part out. The reason is for extra clarity, as a skimmer or someone just checking a specific piece of the doc may otherwise come across the naked 'Window Manager' term and somehow think that it's some component of the engine.

@mhilbrunner
Copy link
Member

Thanks for the review Pedro. @Sauermann Happy to merge this as soon as the above is addressed :)

Update "Viewport and canvas transforms" tutorial

- incldue missing Window-transform
- Update graphic to make the distinction between coordinate systems
and transforms mode clear
- Restructure "transform functions" section
- Update functions to new Godot 4 conventions

Create a more detailed page in engine documentation about 2d coordinate
systems and 2d transforms.
@Sauermann Sauermann force-pushed the fix-transform-2d-update branch from 71bdbe2 to 8111c91 Compare July 19, 2023 17:13
@mhilbrunner mhilbrunner merged commit 689277f into godotengine:master Jul 19, 2023
@Sauermann Sauermann deleted the fix-transform-2d-update branch July 19, 2023 19:25
@mhilbrunner
Copy link
Member

@Sauermann Finally merged, wow this took a while :) All of this should be applicable for all of 4.0, 4.1 and 4.2+, right?

@mhilbrunner mhilbrunner added cherrypick:4.0 cherrypick:4.1 and removed needs work Needs additional work by the original author, someone else or in another repo. labels Jul 19, 2023
@Sauermann
Copy link
Contributor Author

Hehe, doesn't beat my longest waiting PR ;-)
The relevant PRs got merged during 4.0 beta, so I would say yes.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants