Fix scroll to symbol's documentation #100156

ydeltastar · 2024-12-07T22:00:19Z

Fix: Editor help doesn't scroll to where it should #100087

Same fix as #90035 but for a different code path.
A doc page goes through this code path the first time it opens but depending on the device it will execute too early so the correct offsets weren't calculated yet.

passivestar · 2024-12-08T01:05:07Z

Works on my end, thanks

akien-mga · 2024-12-10T16:46:53Z

editor/editor_help.cpp

@@ -215,7 +215,7 @@ void EditorHelp::_search(bool p_search_previous) {

 void EditorHelp::_class_desc_finished() {
 	if (scroll_to >= 0) {
-		class_desc->scroll_to_paragraph(scroll_to);
+		class_desc->connect("draw", callable_mp(class_desc, &RichTextLabel::scroll_to_paragraph).bind(scroll_to), CONNECT_ONE_SHOT | CONNECT_DEFERRED);


Suggested change

class_desc->connect("draw", callable_mp(class_desc, &RichTextLabel::scroll_to_paragraph).bind(scroll_to), CONNECT_ONE_SHOT | CONNECT_DEFERRED);

class_desc->connect(SceneStringName(draw), callable_mp(class_desc, &RichTextLabel::scroll_to_paragraph).bind(scroll_to), CONNECT_ONE_SHOT | CONNECT_DEFERRED);

I wonder if a call_deferred() is enough now, without connect to the draw signal.

Thread can't directly update scrollbars, so it is done in the draw. This change is correct (scrolling after finished and first redraw), but we might want to add another signal instead of directly connecting to the draw (finished probably should not be changed, since other code might expect it to fire even for hidden RTL that will never redraw).

@ydeltastar WDYT about the above suggestion? Do you want to implement that?

Sounds good. I added a signal that triggers after loading, processing, and validation for drawing. It also needed an equivalent method for is_finished() so the EditorHelp could use the signal correctly.

I'm very perplexed on the addition of another signal. It sounds "needlessly" confusing. I'd be wary as developers may use one or the other without understanding the difference.

Yeah, all it does is simulate a one-shot connection to draw. It is very specific, can only happen in the first draw after finished so it could be just a note in the documentation as well.

Maybe you can connect to finished and then in the callback connect to draw?

It's what I did in the first iteration. I reverted the PR to it and added a note to the finished signal.

Yeah, simpler solutions are better. No point in adding a signal if it does the same.

akien-mga · 2024-12-10T16:49:36Z

Same fix as #90035 but for a different code path.

Seems like this was changed further in #96449.

It's starting to look like a game of whac-a-mole....

CC @Maran23 @KoBeWi @kitbdev

Maran23 · 2024-12-10T17:14:57Z

Seems like this was changed further in #96449.

It's starting to look like a game of whac-a-mole....

CC @Maran23 @KoBeWi @kitbdev

I think the problem is in the RTL control, as when it finished, I expect that the scrolling will always work. But it does not.

Why it happens sometimes for the RTL control is not clear, I also start to wonder if this might be related to the refresh rate of the screen as suggested in the issue. I have a 240 Hz screen and never got this issue after my fix again.

My fix removed code that was calling the whole help logic twice, so it fixed a problem, but the scrolling is still not stable (enough). For now, I wonder if simply calling call_deferred() is enough here, doing the scrolling at the end of the frame.

EDIT: What is weird is that the finished signal is called, then the vscroll might get hidden, then queue_redraw is called. I wonder, if we rather want to emit the finished signal after the redraw instead (Or at least after _validate_line_caches is called, which seems to be done right after).

passivestar · 2024-12-10T17:25:11Z

I also start to wonder if this might be related to the refresh rate of the screen as suggested in the issue. I have a 240 Hz screen and never got this issue after my fix again

I use a 120hz one. I get it every time I use search in dev6

ydeltastar · 2024-12-10T17:51:46Z

Seems like this was changed further in #96449.

From what I debugged, #90035 didn't fix the issue because the offsets were already calculated in that code path. Having it or reverting it didn't have any effect. I think it is good to have it connected to draw too as a guarantee anyway.

This PR affects another code path when the documentation is loaded and processed for the first time. When scroll_to_paragraph is called, the offsets aren't calculated yet.

My fix removed code that was calling the whole help logic twice, so it fixed a problem, but the scrolling is still not stable (enough). For now, I wonder if simply calling call_deferred() is enough here, doing the scrolling at the end of the frame.

EDIT: What is weird is that the finished signal is called, then the vscroll might get hidden, then queue_redraw is called. I wonder, if we rather want to emit the finished signal after the redraw instead (Or at least after _validate_line_caches is called, which seems to be done right after).

Yes, the main issue is that RichTextLabel.finished can be emitted before the offsets are calculated in _validate_line_caches which happens in NOTIFICATION_DRAW.

godot/scene/gui/rich_text_label.cpp

Lines 1899 to 1902 in a372214

    
           // Start text shaping. 
        
           if (_validate_line_caches()) { 
        
           	set_physics_process_internal(false); // Disable auto refresh, if text is fully processed. 
        
           } else {

I think it makes sense since these calculations are dependent on theme changes and only relevant when drawing. So connecting to the draw signal which happens after NOTIFICATION_DRAW sounds like the best fix here.

akien-mga · 2024-12-10T17:57:10Z

Note that AFAIK, the RichTextLabel for the editor help updates on a thread, so there might be race conditions. CC @bruvzg

Maran23 · 2024-12-10T18:21:22Z

@passivestar Does CTRL+Left Click works for you?

passivestar · 2024-12-10T18:30:44Z

@passivestar Does CTRL+Left Click works for you?

yeah it does

KoBeWi

Fixes the bug.

Repiteo · 2024-12-30T15:08:20Z

Thanks!

ydeltastar requested a review from a team as a code owner December 7, 2024 22:00

Chaosus added bug topic:editor regression labels Dec 8, 2024

Chaosus added this to the 4.4 milestone Dec 8, 2024

akien-mga reviewed Dec 10, 2024

View reviewed changes

ydeltastar force-pushed the fix-doc-scroll branch 2 times, most recently from bb481c0 to f87ba44 Compare December 18, 2024 00:09

ydeltastar requested review from a team as code owners December 18, 2024 00:09

Fix scroll to symbol's documentation

Loading
Loading status checks…

6478806

ydeltastar force-pushed the fix-doc-scroll branch from f87ba44 to 6478806 Compare December 27, 2024 20:12

KoBeWi approved these changes Dec 27, 2024

View reviewed changes

Repiteo merged commit b987e90 into godotengine:master Dec 30, 2024
20 checks passed

arkology mentioned this pull request Jan 10, 2025

Searching a method via the "Search Help" dialog takes you to the right documentation page, but at the bottom of the page #101360

Closed

ydeltastar deleted the fix-doc-scroll branch March 11, 2025 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scroll to symbol's documentation #100156

Fix scroll to symbol's documentation #100156

ydeltastar commented Dec 7, 2024 •

edited

Loading

passivestar commented Dec 8, 2024

akien-mga Dec 10, 2024

Maran23 Dec 10, 2024

bruvzg Dec 11, 2024

akien-mga Dec 17, 2024

ydeltastar Dec 18, 2024 •

edited

Loading

Mickeon Dec 18, 2024

ydeltastar Dec 18, 2024

KoBeWi Dec 27, 2024

ydeltastar Dec 27, 2024

KoBeWi Dec 27, 2024

akien-mga commented Dec 10, 2024 •

edited

Loading

Maran23 commented Dec 10, 2024 •

edited

Loading

passivestar commented Dec 10, 2024

ydeltastar commented Dec 10, 2024 •

edited

Loading

akien-mga commented Dec 10, 2024

Maran23 commented Dec 10, 2024

passivestar commented Dec 10, 2024

KoBeWi left a comment

Repiteo commented Dec 30, 2024

	class_desc->connect("draw", callable_mp(class_desc, &RichTextLabel::scroll_to_paragraph).bind(scroll_to), CONNECT_ONE_SHOT \| CONNECT_DEFERRED);
	class_desc->connect(SceneStringName(draw), callable_mp(class_desc, &RichTextLabel::scroll_to_paragraph).bind(scroll_to), CONNECT_ONE_SHOT \| CONNECT_DEFERRED);

Fix scroll to symbol's documentation #100156

Fix scroll to symbol's documentation #100156

Conversation

ydeltastar commented Dec 7, 2024 • edited Loading

passivestar commented Dec 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydeltastar Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Dec 10, 2024 • edited Loading

Maran23 commented Dec 10, 2024 • edited Loading

passivestar commented Dec 10, 2024

ydeltastar commented Dec 10, 2024 • edited Loading

akien-mga commented Dec 10, 2024

Maran23 commented Dec 10, 2024

passivestar commented Dec 10, 2024

KoBeWi left a comment

Choose a reason for hiding this comment

Repiteo commented Dec 30, 2024

ydeltastar commented Dec 7, 2024 •

edited

Loading

ydeltastar Dec 18, 2024 •

edited

Loading

akien-mga commented Dec 10, 2024 •

edited

Loading

Maran23 commented Dec 10, 2024 •

edited

Loading

ydeltastar commented Dec 10, 2024 •

edited

Loading