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

Improve documentation of CanvasItem's draw logic #64345

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 12, 2022

This PR changes a lot of the wording to better connect and explain the _draw() and update() methods, the draw signal and the NOTIFICATION_DRAW.

It takes heavy inspiration from how corresponding notifications and signals are worded in the Node documentation.

Here's the gist of why these descriptions have been modified:

  • "For this, update() must be called [...] However, they can only be used inside the Object._notification, signal or _draw() virtual functions.

    • "must be called" implies that it requires user input, but in reality update() is appropriately called by core classes already. The last sentence oddly mentions the notification first and _draw() last, and it ends awkwardly;
  • _draw(): Overridable function called by the engine (if defined) to draw the canvas item.

    • Thanks Captain Obvious for answering how and why, but we'd like to know when is it called? The description has been changed to mention all related methods;
  • update(): Queue the CanvasItem for update. NOTIFICATION_DRAW will be called on idle time to request redraw.

    • Changed to include _draw(), it doing nothing when the CanvasItem is hidden, and noting that NOTIFICATION_DRAW is sent only once per frame, no matter how many update() are called.
  • draw: Emitted when the CanvasItem must redraw. This can only be connected realtime, as deferred will not allow drawing.

    • This is incredibly misleading. Any connected method can and will work just fine. It just will be not allowed to call draw_* methods when deferred;

I wanted to mention more often that most of this doesn't work when CanvasItem is not visible, but I wasn't sure how in a way that feels right.

This can be cherrypicked to 3.x.

@Mickeon Mickeon requested a review from a team as a code owner August 12, 2022 23:26
@YuriSizov YuriSizov added this to the 4.x milestone Aug 12, 2022
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 13, 2022

Question, why is this scheduled for 4.x?

@YuriSizov
Copy link
Contributor

To keep the 4.0 milestone clear after the roadmap freeze that we've announced two weeks ago. It can still be merged in 4.0, if it's reviewed and approved. But it doesn't seem to be essential for the 4.0 release.

@mhilbrunner
Copy link
Member

mhilbrunner commented Aug 17, 2022

Thanks for working on this! The docs/class reference can really use more of this. A lot of meat is already there, but often the nuance/details you actually care about when looking at it as a gamedev can be improved.

I wanted to mention more often that most of this doesn't work when CanvasItem is not visible, but I wasn't sure how in a way that feels right.

TBH I'm not against adding a 'Note: This won't be called while the CanvasItem is not visible.' at the end of each affected method.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 17, 2022

TBH I'm not against adding a 'Note: This won't be called while the CanvasItem is not visible.' at the end of each affected method

I found it difficult to find the right balance. Too many details, especially if repetitive, and it does definitely feel overbearing to the user (that may not ever need this much definition). Too little, and it ends up looking like "Overridable function called by the engine (if defined) to draw the canvas item", although this one is particularly egregious, 'cause the user likely doesn't care about this.

Reality is, update() is called completely fine regardless of visibility. However, by the end of the frame, if the CanvasItem is still hidden, NOTIFICATION_DRAW is not sent. By extension, _draw() isn't called and "draw" is not emitted. All three of which practically summarise their behaviour in this PR with " when requested to draw".

@clayjohn clayjohn added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 18, 2022
@clayjohn clayjohn merged commit de083c8 into godotengine:master Aug 18, 2022
@clayjohn
Copy link
Member

Looks great to me, thanks!

@Mickeon Mickeon deleted the docs-better-draw branch August 19, 2022 10:03
@akien-mga akien-mga modified the milestones: 4.x, 4.0 Aug 24, 2022
@akien-mga akien-mga added the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 24, 2022
@akien-mga
Copy link
Member

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 24, 2022
@akien-mga
Copy link
Member

Cherry-picked for 3.5.1.

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 29, 2022
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.

5 participants