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

Rewrite most of Resource's documentation #67072

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Oct 8, 2022

I swear, I totally didn't spend 2 days on this PR, which touches up a lot of Resource's documentation. Despite being a very important class, it was smelling very old, at least to me. And I was angry.

The documentation has been "modernised", based on the way other classes are written (especially Node). I personally verified many concepts extensively, by looking at the source code and through testing. The changes are so numerous that it's difficult to summarise them all without calling them "improvements".

Do note that whenever I say "user" or "users", I do mean the average, possibly a bit inexperienced person, learning the engine.

The most notable changes and their reasoning include:

  • duplicate has been shortened and corrected.

    • Despite being such a simple concept, the description felt compelled to convolute the basic functionality of the very vague parameter name. So much, in fact, that it may have been entirely wrong.
  • Point out that resource_local_to_scene duplicates the original resource at run-time.

    • This is what happens behind the s c e n e s. Let's not pretend it's not the case by attempting to introduce a "local"/"global" concept to it. Otherwise, users may think that changing the original Resource also affects the "local" Resources at run-time.
  • Added a more accurate explanation to setup_local_to_scene and given an example.

    • I don't know why this method is exposed, but it was very vague before. When was the method called by the engine? What resource called the method (original Resource or its duplicate)? It probably could be shortened, but it needs to be more accurate and verbose, because the user is also able to call it. As simple as it is, they need to know what it does.
    • Honestly, this is the method I spent the most hours on, attempting so many iterations to explain the same concept, and that is not a good sign to me. Deprecate Resource.setup_local_to_scene #67082
  • Moved that one note of what ViewportTexture does in setup_local_to_scene to ViewportTexture itself.

    • Most users do not care about this, nor they need to. In fact, I would've removed it outright, but because the method is exposed, users need to know it is happening.
  • Added more details and a simple example for using emit_changed.

    • It was missing, despite being part of the common workflow.

I'm aware this is touchy. Feedback is very, very welcome.

doc/classes/Resource.xml Outdated Show resolved Hide resolved
@mhilbrunner mhilbrunner merged commit f77f7d4 into godotengine:master Oct 21, 2022
@mhilbrunner
Copy link
Member

Great work, thank you!

@Mickeon
Copy link
Contributor Author

Mickeon commented Oct 21, 2022

Thank you very much!

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.

4 participants