Fix stretched/misplaced logo on startup & race conditions#1286
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds timed worker shutdown coordination across editor and engine paths, and updates boot rendering to account for compositor-driven window sizing. Wayland and X11 now synchronize initial configuration and resize events before boot-image redraws. ChangesTimeout-bounded thread shutdown synchronization
Compositor sync and boot splash sizing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Main
participant DisplayServer
participant WaylandThread
participant BootRenderer
Main->>DisplayServer: process_events()
Main->>BootRenderer: setup_boot_logo()
BootRenderer->>DisplayServer: pump_resize_events()
DisplayServer->>WaylandThread: process configure/resize events
WaylandThread-->>DisplayServer: updated window dimensions
DisplayServer-->>BootRenderer: refreshed window size
BootRenderer->>BootRenderer: recompute and redraw boot image
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GeneralProtectionFault
force-pushed
the
splash_wayland
branch
2 times, most recently
from
52acf13 to
c85fba8
Compare
|
@GeneralProtectionFault What is the status of this PR, can we open it for review now? |
Yeah I'll ready it up. We know it works, I just felt a little uneasy about knowing if this touches somewhere I'm not aware of, etc... But, no harm I guess, let's see what the High Hare has to say about it. Ah....lemme git it synced up too. |
GeneralProtectionFault
force-pushed
the
splash_wayland
branch
from
08413f2 to
0d65b5c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
editor/doc/editor_help.cpp (1)
3162-3188: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winSignal
_worker_thread_doneon every thread exit path.
_worker_thread_doneis reset before this thread starts, but the error/corrupt-cache returns exit before Line 3188.cleanup_doc()can then wait forever on Line 3421.Proposed fix
void EditorHelp::_load_script_doc_cache_thread(void *p_udata) { - ERR_FAIL_COND_MSG(!ProjectSettings::get_singleton()->is_project_loaded(), "Error: cannot load script doc cache without a project."); - ERR_FAIL_COND_MSG(!ResourceLoader::exists(get_script_doc_cache_full_path()), "Error: cannot load script doc cache from inexistent file."); + if (!ProjectSettings::get_singleton()->is_project_loaded()) { + _worker_thread_done.set(); + ERR_FAIL_MSG("Error: cannot load script doc cache without a project."); + } + if (!ResourceLoader::exists(get_script_doc_cache_full_path())) { + _worker_thread_done.set(); + ERR_FAIL_MSG("Error: cannot load script doc cache from inexistent file."); + } Ref<Resource> script_doc_cache_res = ResourceLoader::load(get_script_doc_cache_full_path(), "", ResourceFormatLoader::CACHE_MODE_IGNORE); if (script_doc_cache_res.is_null()) { print_verbose("Script doc cache is corrupted. Regenerating it instead."); _delete_script_doc_cache(); callable_mp_static(EditorHelp::regenerate_script_doc_cache).call_deferred(); + _worker_thread_done.set(); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/doc/editor_help.cpp` around lines 3162 - 3188, EditorHelp::_load_script_doc_cache_thread currently returns early on the failed load/corrupt-cache path without signaling _worker_thread_done, which can leave cleanup_doc() waiting indefinitely. Update this function so every exit path, including the null ResourceLoader::load case and any other failure returns, sets _worker_thread_done before returning; keep the normal success path signaling as well.editor/inspector/editor_resource_preview.cpp (1)
606-616: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the timeout affect the join path too.
After breaking on timeout, Line 616 still blocks indefinitely in
wait_to_finish(), so this doesn’t actually bound shutdown. If skipping the join is unsafe, keep waiting and treat this as a warning threshold instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/inspector/editor_resource_preview.cpp` around lines 606 - 616, The timeout in EditorResourcePreview’s preview-thread loop only stops the polling loop, but thread.wait_to_finish() still blocks forever afterward. Update the shutdown flow so the same TIMEOUT_USEC limit also applies to the join path: either avoid calling wait_to_finish() after timeout, or keep waiting while treating the timeout as a warning threshold rather than a hard exit. Use the existing exited flag, TIMEOUT_USEC check, and thread.wait_to_finish() in EditorResourcePreview to make the behavior consistent.
🧹 Nitpick comments (2)
drivers/gles3/rasterizer_gles3.cpp (1)
491-502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate boot-rect scaling logic across renderers.
This same scale/center computation (screen_get_scale multiply + floor-centered offset) is duplicated verbatim in
RendererCompositorRD::set_boot_image(servers/rendering/renderer_rd/renderer_compositor_rd.cpp, lines 227-234). Consider factoring this into a shared helper (e.g., alongsideOS::calculate_boot_screen_rect) to avoid the two implementations drifting apart in future fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/gles3/rasterizer_gles3.cpp` around lines 491 - 502, The boot-image screen rect scaling and centering logic in rasterizer_gles3.cpp is duplicated in RendererCompositorRD::set_boot_image, so update this path to use a shared helper instead of maintaining two copies. Factor the screen_get_scale multiplication and floor-centered offset computation into a common function near OS::calculate_boot_screen_rect, then have both RasterizerGLES3 and RendererCompositorRD call it so the behavior stays consistent.platform/linuxbsd/x11/display_server_x11.cpp (1)
1942-1955: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSleeping up to 100ms while holding
events_mutex.The fallback polling loop calls
OS::get_singleton()->delay_usec(5000)up to 20 times inside theMutexLock mutex_lock(events_mutex);scope opened at line 1928. This can block the dedicated_poll_events_threadfrom touchingpolled_eventsfor up to 100ms during startup. Bounded and one-shot (MAIN_WINDOW_ID only), so impact is limited, but narrowing the lock to just theXCheckTypedWindowEventcheck (re-acquiring it each iteration) would avoid holding it across the sleep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/linuxbsd/x11/display_server_x11.cpp` around lines 1942 - 1955, The fallback polling in display_server_x11.cpp holds events_mutex while calling OS::get_singleton()->delay_usec(5000), which can block _poll_events_thread from accessing polled_events during startup. Update the logic around the ConfigureNotify wait in the X11 window setup path so the mutex is only held for the XCheckTypedWindowEvent/_window_changed access, and is released before any sleep/delay in the loop. Keep the change localized to the startup polling block that uses get_config_event, wd.maximized, wd.fullscreen, and MAIN_WINDOW_ID behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/object/worker_thread_pool.cpp`:
- Around line 861-884: The shutdown wait loops in WorkerThreadPool’s cleanup
path currently break on timeout and then continue teardown, which can leave
worker threads running against stale SceneTree/script state. Update the timeout
handling in the pre-exit and exit-language wait sections so a timeout does not
fall through to normal shutdown; instead, fail loudly and stop the remaining
shutdown sequence, or otherwise escalate in a way that prevents Main::cleanup()
from deleting scene/script resources while threads may still be active. Use the
existing _switch_runlevel, control_cond_var.wait_for_usec, and WARN_PRINT flow
to locate and adjust the shutdown logic.
In `@editor/file_system/editor_file_system.cpp`:
- Around line 1736-1748: The scan timeout path in
EditorFileSystem::reconnect_scan_thread currently skips waiting for the worker
and can free filesystem state while the scan thread is still active. Update the
timeout handling around active_thread/scanning so any shutdown or cleanup that
touches filesystem and new_filesystem only happens after the scan thread has
fully stopped, either by always joining via wait_to_finish() or by otherwise
ensuring the thread is detached and never accesses those members again. Apply
the same fix to the later cleanup block that deletes filesystem/new_filesystem
so the thread lifecycle and state teardown remain synchronized.
In `@editor/scene/2d/tiles/tiles_editor_plugin.cpp`:
- Around line 360-370: The timeout in TilesEditorUtils teardown is ineffective
because pattern_preview_thread.wait_to_finish() still blocks after the wait loop
breaks. Update the shutdown path around pattern_thread_exited, TIMEOUT_USEC, and
pattern_preview_thread so the worker is guaranteed to exit before joining, or
convert the timeout into a warning-only threshold and continue waiting until the
thread finishes. Keep the fix localized to the destructor/cleanup logic in
tiles_editor_plugin.cpp and preserve the existing warning behavior.
In `@platform/linuxbsd/wayland/wayland_thread.cpp`:
- Around line 4891-4904: `WaylandThread::wait_window_rect_ms` reads shared
window state before taking `mutex`, creating the same race that
`wait_frame_suspend_ms` avoids. Move the `windows.has(p_id)` check and the
`windows[p_id].rect.size` snapshot to after `MutexLock mutex_lock(mutex);`, so
all accesses to `windows` and `rect.size` happen under the lock before calling
`wl_display_roundtrip`.
- Around line 1440-1441: The xdg configure handlers are not marking the initial
configure as complete, so the non-libdecor path keeps retrying through all
MAX_CONFIGURE_ROUNDTRIPS and hits the warning on first window creation. Update
the xdg configure flow in _xdg_surface_on_configure() and/or
_xdg_toplevel_on_configure() to set ws->initial_configure_done when the initial
configure is received, and remove the duplicated wl_display_roundtrip comment
from that area.
In `@platform/linuxbsd/x11/display_server_x11.cpp`:
- Around line 2005-2006: The show_window verbose log statement in
display_server_x11.cpp is missing its terminating semicolon, causing a compile
error. Update the show_window path in display_server_x11::... where
print_verbose(vformat("show_window: final size %s", wd.size)) appears so the
statement is properly terminated with a semicolon and the function body compiles
cleanly.
---
Outside diff comments:
In `@editor/doc/editor_help.cpp`:
- Around line 3162-3188: EditorHelp::_load_script_doc_cache_thread currently
returns early on the failed load/corrupt-cache path without signaling
_worker_thread_done, which can leave cleanup_doc() waiting indefinitely. Update
this function so every exit path, including the null ResourceLoader::load case
and any other failure returns, sets _worker_thread_done before returning; keep
the normal success path signaling as well.
In `@editor/inspector/editor_resource_preview.cpp`:
- Around line 606-616: The timeout in EditorResourcePreview’s preview-thread
loop only stops the polling loop, but thread.wait_to_finish() still blocks
forever afterward. Update the shutdown flow so the same TIMEOUT_USEC limit also
applies to the join path: either avoid calling wait_to_finish() after timeout,
or keep waiting while treating the timeout as a warning threshold rather than a
hard exit. Use the existing exited flag, TIMEOUT_USEC check, and
thread.wait_to_finish() in EditorResourcePreview to make the behavior
consistent.
---
Nitpick comments:
In `@drivers/gles3/rasterizer_gles3.cpp`:
- Around line 491-502: The boot-image screen rect scaling and centering logic in
rasterizer_gles3.cpp is duplicated in RendererCompositorRD::set_boot_image, so
update this path to use a shared helper instead of maintaining two copies.
Factor the screen_get_scale multiplication and floor-centered offset computation
into a common function near OS::calculate_boot_screen_rect, then have both
RasterizerGLES3 and RendererCompositorRD call it so the behavior stays
consistent.
In `@platform/linuxbsd/x11/display_server_x11.cpp`:
- Around line 1942-1955: The fallback polling in display_server_x11.cpp holds
events_mutex while calling OS::get_singleton()->delay_usec(5000), which can
block _poll_events_thread from accessing polled_events during startup. Update
the logic around the ConfigureNotify wait in the X11 window setup path so the
mutex is only held for the XCheckTypedWindowEvent/_window_changed access, and is
released before any sleep/delay in the loop. Keep the change localized to the
startup polling block that uses get_config_event, wd.maximized, wd.fullscreen,
and MAIN_WINDOW_ID behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 843d9ed9-d56d-4c1c-a946-394739f82eb2
📒 Files selected for processing (18)
core/object/worker_thread_pool.cppcore/os/condition_variable.hdrivers/gles3/rasterizer_gles3.cppeditor/doc/editor_help.cppeditor/doc/editor_help.heditor/file_system/editor_file_system.cppeditor/inspector/editor_resource_preview.cppeditor/scene/2d/tiles/tiles_editor_plugin.cppeditor/scene/2d/tiles/tiles_editor_plugin.hmain/main.cppplatform/linuxbsd/wayland/display_server_wayland.cppplatform/linuxbsd/wayland/display_server_wayland.hplatform/linuxbsd/wayland/wayland_thread.cppplatform/linuxbsd/wayland/wayland_thread.hplatform/linuxbsd/x11/display_server_x11.cppplatform/linuxbsd/x11/display_server_x11.hservers/display_server.hservers/rendering/renderer_rd/renderer_compositor_rd.cpp
| const uint64_t TIMEOUT_USEC = 5000000; // 5 seconds per phase | ||
|
|
||
| MutexLock lock(task_mutex); | ||
|
|
||
| // Wait until all threads are idle. | ||
| _switch_runlevel(RUNLEVEL_PRE_EXIT_LANGUAGES); | ||
| while (runlevel_data.pre_exit_languages.num_idle_threads != threads.size()) { | ||
| control_cond_var.wait(lock); | ||
| bool notified = control_cond_var.wait_for_usec(lock, TIMEOUT_USEC); | ||
| if (!notified && runlevel_data.pre_exit_languages.num_idle_threads != threads.size()) { | ||
| WARN_PRINT("WorkerThreadPool: Timed out waiting for threads to become idle during shutdown. " | ||
| "Some tasks may have been abandoned."); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Wait until all threads have detached from scripting languages. | ||
| _switch_runlevel(RUNLEVEL_EXIT_LANGUAGES); | ||
| while (runlevel_data.exit_languages.num_exited_threads != threads.size()) { | ||
| control_cond_var.wait(lock); | ||
| bool notified = control_cond_var.wait_for_usec(lock, TIMEOUT_USEC); | ||
| if (!notified && runlevel_data.exit_languages.num_exited_threads != threads.size()) { | ||
| WARN_PRINT("WorkerThreadPool: Timed out waiting for threads to exit scripting languages."); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Timeout fallback undermines the shutdown-safety guarantee it's paired with.
Breaking out of the wait loop on timeout still lets the function return normally and move to the next runlevel (and eventually back to Main::cleanup(), which now deletes the main loop/SceneTree right after this call). If a worker task is genuinely stuck or just slow (>5s, or >10s across both phases), a thread can still be executing with stale scene/script references at the moment the SceneTree is deleted and ScriptServer::finish_languages() runs — the exact use-after-free/deadlock scenario the new call ordering in Main::cleanup() is meant to avoid. The timeout converts a hang into a (rarer but still possible) crash during shutdown.
Consider making the timeout path fail loud rather than silently proceed — e.g., skip forcing RUNLEVEL_EXIT_LANGUAGES/subsequent shutdown steps when the previous phase timed out, or escalate (abort/force-terminate stuck threads) instead of falling through to normal teardown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/object/worker_thread_pool.cpp` around lines 861 - 884, The shutdown wait
loops in WorkerThreadPool’s cleanup path currently break on timeout and then
continue teardown, which can leave worker threads running against stale
SceneTree/script state. Update the timeout handling in the pre-exit and
exit-language wait sections so a timeout does not fall through to normal
shutdown; instead, fail loudly and stop the remaining shutdown sequence, or
otherwise escalate in a way that prevents Main::cleanup() from deleting
scene/script resources while threads may still be active. Use the existing
_switch_runlevel, control_cond_var.wait_for_usec, and WARN_PRINT flow to locate
and adjust the shutdown logic.
GeneralProtectionFault
force-pushed
the
splash_wayland
branch
from
0d65b5c to
6d0f67d
Compare
|
I am planning to look through the issues with this thing when I get a chance. I think the gross oversimplification is race conditions are part of it, and fixing them must heed dominoes. I observed the engine hanging on close (before any changes), similar to #867, so it's got tentacles. The diagram from the bunny is very helpful, so I'll see if I can straighten this out without too much of the engine noticing. |
|
Hi, thanks for looking into this. I gave 6d0f67d a quick try on hyprland, and it does indeed fix the boot splash scaling. |
Sorry, this is my first time using Hyperland so I'm not overly familiar with it. What do you mean by wm-tile? Window manager? Jon was testing resizing the (floating) window during the startup and it does not adjust to that, if that's what you mean? On a standard desktop, I can't budge it at all if I try that, and I think we can expect that, the UI isn't responsive there, I think the main thread is busy at that moment and nothing's gonna budge that without seriously redesigning a more responsive UI. If that's not what you mean, though, just fill me in. |
|
Hi, sorry - yes the window manager tile for the window. |
GeneralProtectionFault
force-pushed
the
splash_wayland
branch
from
6d0f67d to
d3b37aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
drivers/gles3/rasterizer_gles3.cpp (1)
491-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover dead comment; core scaling refactor otherwise looks correct.
Line 492's commented-out
Size2 win_size_f = ...note is now stale sincecalculate_boot_screen_rect/calculate_boot_image_recthandle theSize2i→Size2conversion internally. Consider removing it. The delegation to the two centralized helpers itself (matchingrenderer_compositor_rd.cpp) is a good consolidation and looks correct.🧹 Suggested cleanup
Rect2 imgrect(0, 0, p_image->get_width(), p_image->get_height()); - // Size2 win_size_f = Size2(win_size); // This is needed for the .floor() function below because Size2i does not have a floor() function (but Size2 does) Rect2 screenrect;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/gles3/rasterizer_gles3.cpp` around lines 491 - 499, Remove the stale commented-out Size2 win_size_f note in rasterizer_gles3::... around the imgrect/screenrect setup, since the Size2i to Size2 conversion is now handled inside calculate_boot_screen_rect and calculate_boot_image_rect. Keep the current delegation to OS::get_singleton()->calculate_boot_screen_rect and DisplayServer::calculate_boot_image_rect unchanged, and just clean up the leftover dead comment near the screenrect initialization.core/os/os.h (1)
174-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
OS::calculate_boot_image_rectdeclaration
DisplayServer::calculate_boot_image_rectalready owns this helper, and all current callers use that version. Keeping theOSdeclaration only duplicates the API and adds confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/os/os.h` around lines 174 - 176, Remove the unused OS::calculate_boot_image_rect declaration from the OS class interface, since DisplayServer::calculate_boot_image_rect is the active helper and current callers already use it. Update the OS header to drop this redundant API entry and make sure any references in nearby code still point to DisplayServer::calculate_boot_image_rect so there is only one boot image rect helper exposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/linuxbsd/wayland/wayland_thread.cpp`:
- Around line 4895-4897: The size-change check in
window_state_update_size()/compositor_sync() is comparing only
WindowState::rect.size, so a pure scale change can be missed and
process_events() gets skipped. Update the comparison to use the window’s
scaled/physical size when deciding whether nothing changed, and apply the same
fix to the related checks in the nearby window-state update paths so
WindowRectMessage enqueues still trigger event processing. Use the existing
window_state_update_size, compositor_sync, and windows[p_id] logic to locate and
adjust the comparison.
In `@platform/linuxbsd/x11/display_server_x11.cpp`:
- Around line 1961-2011: The final sizing logic in display_server_x11.cpp is
outside the MAIN_WINDOW_ID handling scope, which breaks compilation and leaves
show_window() braces mismatched. Move the maximize/fullscreen size estimation
block back inside the MAIN_WINDOW_ID branch in DisplayServerX11::show_window,
and make sure the closing braces keep the print_verbose call inside the function
rather than ending show_window() early.
In `@servers/display_server.h`:
- Line 368: Remove the stale OS-side declaration for calculate_boot_image_rect
and keep the API owned by DisplayServer. Update the OS header that still exposes
OS::calculate_boot_image_rect so it no longer advertises an unused helper, and
ensure any references remain on DisplayServer::calculate_boot_image_rect since
that is the only implementation used by current call sites.
---
Nitpick comments:
In `@core/os/os.h`:
- Around line 174-176: Remove the unused OS::calculate_boot_image_rect
declaration from the OS class interface, since
DisplayServer::calculate_boot_image_rect is the active helper and current
callers already use it. Update the OS header to drop this redundant API entry
and make sure any references in nearby code still point to
DisplayServer::calculate_boot_image_rect so there is only one boot image rect
helper exposed.
In `@drivers/gles3/rasterizer_gles3.cpp`:
- Around line 491-499: Remove the stale commented-out Size2 win_size_f note in
rasterizer_gles3::... around the imgrect/screenrect setup, since the Size2i to
Size2 conversion is now handled inside calculate_boot_screen_rect and
calculate_boot_image_rect. Keep the current delegation to
OS::get_singleton()->calculate_boot_screen_rect and
DisplayServer::calculate_boot_image_rect unchanged, and just clean up the
leftover dead comment near the screenrect initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b2454b1-6f3a-4002-870d-b738611e89f3
📒 Files selected for processing (20)
core/object/worker_thread_pool.cppcore/os/condition_variable.hcore/os/os.hdrivers/gles3/rasterizer_gles3.cppeditor/doc/editor_help.cppeditor/doc/editor_help.heditor/file_system/editor_file_system.cppeditor/inspector/editor_resource_preview.cppeditor/scene/2d/tiles/tiles_editor_plugin.cppeditor/scene/2d/tiles/tiles_editor_plugin.hmain/main.cppplatform/linuxbsd/wayland/display_server_wayland.cppplatform/linuxbsd/wayland/display_server_wayland.hplatform/linuxbsd/wayland/wayland_thread.cppplatform/linuxbsd/wayland/wayland_thread.hplatform/linuxbsd/x11/display_server_x11.cppplatform/linuxbsd/x11/display_server_x11.hservers/display_server.cppservers/display_server.hservers/rendering/renderer_rd/renderer_compositor_rd.cpp
✅ Files skipped from review due to trivial changes (3)
- editor/scene/2d/tiles/tiles_editor_plugin.h
- core/object/worker_thread_pool.cpp
- editor/file_system/editor_file_system.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- platform/linuxbsd/x11/display_server_x11.h
- platform/linuxbsd/wayland/display_server_wayland.h
- editor/inspector/editor_resource_preview.cpp
- editor/scene/2d/tiles/tiles_editor_plugin.cpp
- editor/doc/editor_help.h
- main/main.cpp
- core/os/condition_variable.h
- platform/linuxbsd/wayland/display_server_wayland.cpp
- platform/linuxbsd/wayland/wayland_thread.h
- editor/doc/editor_help.cpp
| ERR_FAIL_COND_V(!windows.has(p_id), false); | ||
| Size2i initial_size = windows[p_id].rect.size; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare the scaled window size before deciding nothing changed.
window_state_update_size() can enqueue a WindowRectMessage when only scale changes, but this wait compares only the unscaled WindowState::rect.size. In that case compositor_sync() returns false and skips process_events(), leaving the boot logo based on stale physical dimensions.
Suggested fix
ERR_FAIL_COND_V(!windows.has(p_id), false);
- Size2i initial_size = windows[p_id].rect.size;
+ WindowState &ws = windows[p_id];
+ auto get_current_size = [&]() {
+ return scale_vector2i(ws.rect.size, window_state_get_scale_factor(&ws));
+ };
+ Size2i initial_size = get_current_size();
wl_display_roundtrip(wl_display);
- if (windows[p_id].rect.size != initial_size) {
+ if (get_current_size() != initial_size) {
return true; // Already changed during the roundtrip.
}
@@
- if (windows[p_id].rect.size != initial_size) {
+ if (get_current_size() != initial_size) {
return true;
}
@@
- if (windows[p_id].rect.size != initial_size) {
+ if (get_current_size() != initial_size) {
return true;
}Also applies to: 4916-4918, 4933-4935
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/linuxbsd/wayland/wayland_thread.cpp` around lines 4895 - 4897, The
size-change check in window_state_update_size()/compositor_sync() is comparing
only WindowState::rect.size, so a pure scale change can be missed and
process_events() gets skipped. Update the comparison to use the window’s
scaled/physical size when deciding whether nothing changed, and apply the same
fix to the related checks in the nearby window-state update paths so
WindowRectMessage enqueues still trigger event processing. Use the existing
window_state_update_size, compositor_sync, and windows[p_id] logic to locate and
adjust the comparison.
| virtual Rect2i screen_get_usable_rect(int p_screen = SCREEN_OF_MAIN_WINDOW) const = 0; | ||
| virtual int screen_get_dpi(int p_screen = SCREEN_OF_MAIN_WINDOW) const = 0; | ||
| virtual float screen_get_scale(int p_screen = SCREEN_OF_MAIN_WINDOW) const; | ||
| static Rect2 calculate_boot_image_rect(const Size2 &p_window_size, const Rect2 &p_imgrect); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether OS::calculate_boot_image_rect has a definition or any callers.
rg -n 'calculate_boot_image_rect' --type=cpp -C2Repository: Redot-Engine/redot-engine
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== matches ==\n'
rg -n 'calculate_boot_image_rect' .
printf '\n== relevant headers ==\n'
sed -n '150,190p' core/os/os.h
printf '\n--- display_server.h ---\n'
sed -n '350,385p' servers/display_server.h
printf '\n== likely implementation area ==\n'
sed -n '560,610p' servers/display_server.cppRepository: Redot-Engine/redot-engine
Length of output: 6734
Remove the stale OS::calculate_boot_image_rect declaration
core/os/os.h still declares this helper, but only DisplayServer defines it and all current call sites use DisplayServer::calculate_boot_image_rect. Remove the unused OS declaration, or add a real OS implementation if that’s the intended API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@servers/display_server.h` at line 368, Remove the stale OS-side declaration
for calculate_boot_image_rect and keep the API owned by DisplayServer. Update
the OS header that still exposes OS::calculate_boot_image_rect so it no longer
advertises an unused helper, and ensure any references remain on
DisplayServer::calculate_boot_image_rect since that is the only implementation
used by current call sites.
d3b37aa to
970a847
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform/linuxbsd/wayland/display_server_wayland.cpp (1)
786-806: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmit the rect callback after the compositor size sync
rect_changed_callbackfires beforewd.rect.sizeis updated, soWindow::_rect_changed_callback()consumes the pre-sync size and stays out of sync with the actual surface dimensions until another resize event arrives. Re-fire it after the resync, or move the callback below the size update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/linuxbsd/wayland/display_server_wayland.cpp` around lines 786 - 806, The rect changed callback is being emitted before the compositor-provided physical size is applied, so Window::_rect_changed_callback() sees the old wd.rect size. In display_server_wayland.cpp, update the size sync logic in the show_window path first using the WindowState/physical_size calculation, then invoke wd.rect_changed_callback.call(wd.rect) after wd.rect.size has been updated, or move the existing callback block below that sync so the callback observes the final surface dimensions.
🧹 Nitpick comments (1)
main/main.cpp (1)
3557-3557: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the commented-out
process_events()call.Leaving disabled code with no explanation invites confusion about whether event-syncing here is intended. Either remove it or add a note describing why it must stay disabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main/main.cpp` at line 3557, Remove the commented-out DisplayServer::get_singleton()->process_events() call from main/main.cpp, or replace it with a brief explanatory comment if it must remain disabled. Update the nearby main loop code so the intent is clear and there is no dead code left behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/linuxbsd/wayland/display_server_wayland.cpp`:
- Around line 802-804: The verbose log in display_server_wayland.cpp’s
show_window path is being formed with a backslash line continuation, which folds
indentation tabs into the emitted message. Update the print_verbose/vformat call
to use adjacent string-literal concatenation instead of a continued literal, so
the message text stays clean while preserving the same compositor physical size
context.
In `@platform/linuxbsd/x11/display_server_x11.cpp`:
- Around line 1944-1960: The retry loop in display_server_x11.cpp can miss a
pending ConfigureNotify because it only checks XCheckTypedWindowEvent and never
re-scans polled_events, so the size update may be delayed unnecessarily. Update
the polling logic in the retry block that uses get_config_event, events_mutex,
and _window_changed to also inspect polled_events for a queued ConfigureNotify
on wd.x11_window before sleeping, and apply the same window-change handling when
found.
---
Outside diff comments:
In `@platform/linuxbsd/wayland/display_server_wayland.cpp`:
- Around line 786-806: The rect changed callback is being emitted before the
compositor-provided physical size is applied, so
Window::_rect_changed_callback() sees the old wd.rect size. In
display_server_wayland.cpp, update the size sync logic in the show_window path
first using the WindowState/physical_size calculation, then invoke
wd.rect_changed_callback.call(wd.rect) after wd.rect.size has been updated, or
move the existing callback block below that sync so the callback observes the
final surface dimensions.
---
Nitpick comments:
In `@main/main.cpp`:
- Line 3557: Remove the commented-out
DisplayServer::get_singleton()->process_events() call from main/main.cpp, or
replace it with a brief explanatory comment if it must remain disabled. Update
the nearby main loop code so the intent is clear and there is no dead code left
behind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35313032-fbb3-4fa0-874e-f3feb4ab504b
📒 Files selected for processing (19)
core/object/worker_thread_pool.cppcore/os/condition_variable.hcore/os/os.hdrivers/gles3/rasterizer_gles3.cppeditor/doc/editor_help.cppeditor/doc/editor_help.heditor/file_system/editor_file_system.cppeditor/inspector/editor_resource_preview.cppeditor/scene/2d/tiles/tiles_editor_plugin.cppeditor/scene/2d/tiles/tiles_editor_plugin.hmain/main.cppplatform/linuxbsd/wayland/display_server_wayland.cppplatform/linuxbsd/wayland/display_server_wayland.hplatform/linuxbsd/wayland/wayland_thread.cppplatform/linuxbsd/wayland/wayland_thread.hplatform/linuxbsd/x11/display_server_x11.cppservers/display_server.cppservers/display_server.hservers/rendering/renderer_rd/renderer_compositor_rd.cpp
✅ Files skipped from review due to trivial changes (4)
- editor/file_system/editor_file_system.cpp
- core/object/worker_thread_pool.cpp
- editor/scene/2d/tiles/tiles_editor_plugin.h
- platform/linuxbsd/wayland/display_server_wayland.h
🚧 Files skipped from review as they are similar to previous changes (11)
- editor/doc/editor_help.h
- drivers/gles3/rasterizer_gles3.cpp
- editor/inspector/editor_resource_preview.cpp
- servers/display_server.cpp
- core/os/os.h
- core/os/condition_variable.h
- servers/rendering/renderer_rd/renderer_compositor_rd.cpp
- editor/scene/2d/tiles/tiles_editor_plugin.cpp
- servers/display_server.h
- platform/linuxbsd/wayland/wayland_thread.h
- editor/doc/editor_help.cpp
GeneralProtectionFault
force-pushed
the
splash_wayland
branch
from
970a847 to
a41e5b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
platform/linuxbsd/x11/display_server_x11.cpp (1)
2717-2739: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed 16ms delay is a single-shot heuristic; consider a bounded retry loop.
Unlike
WaylandThread::window_create()'s retry loop (up toMAX_CONFIGURE_ROUNDTRIPS), this waits a single fixed 16ms then drains once. Under a loaded compositor (the exact "Hyprland under load" scenario called out in the comment), theConfigureNotifymay still arrive after this single wait, causing the boot-image resize redraw to be skipped for that launch.♻️ Suggested loop
- OS::get_singleton()->delay_usec(16000); // ~1 frame @ 60 fps - XSync(x11_display, False); - - { - MutexLock mutex_lock(events_mutex); - for (uint32_t i = 0; i < polled_events.size(); ++i) { - if (polled_events[i].type == ConfigureNotify) { - _window_changed(&polled_events[i]); - } - } - XEvent ev; - while (XCheckTypedEvent(x11_display, ConfigureNotify, &ev)) { - _window_changed(&ev); - } - } + const int MAX_RESIZE_POLLS = 3; + for (int i = 0; i < MAX_RESIZE_POLLS; i++) { + OS::get_singleton()->delay_usec(16000); // ~1 frame @ 60 fps + XSync(x11_display, False); + + MutexLock mutex_lock(events_mutex); + bool got_event = false; + for (uint32_t j = 0; j < polled_events.size(); ++j) { + if (polled_events[j].type == ConfigureNotify) { + _window_changed(&polled_events[j]); + got_event = true; + } + } + XEvent ev; + while (XCheckTypedEvent(x11_display, ConfigureNotify, &ev)) { + _window_changed(&ev); + got_event = true; + } + if (got_event) { + break; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/linuxbsd/x11/display_server_x11.cpp` around lines 2717 - 2739, Update DisplayServerX11::pump_resize_events to replace the single 16ms delay and drain with a bounded retry loop, similar to WaylandThread::window_create() and using MAX_CONFIGURE_ROUNDTRIPS where appropriate. Each iteration should wait one frame interval, synchronize, and process pending ConfigureNotify events, while preserving the existing mutex protection and avoiding unbounded waiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drivers/gles3/rasterizer_gles3.cpp`:
- Around line 536-551: Replace the duplicated scaling and centering logic in the
screenrect2 construction with the existing shared helper used above, such as
OS::calculate_boot_screen_rect() or DisplayServer::calculate_boot_image_rect().
Preserve the current p_scale behavior and reuse the helper’s result rather than
maintaining independent Rect2 calculations.
In `@editor/doc/editor_help.h`:
- Around line 207-209: Change _cleanup_in_progress from a plain bool to
SafeFlag, matching _worker_thread_done and _loader_thread_done. Update its
initialization and any reads or writes in cleanup_doc() and related
worker/loader paths to use the existing SafeFlag interface consistently.
In `@platform/linuxbsd/x11/display_server_x11.cpp`:
- Around line 1962-1965: Add the missing statement terminator to the
print_verbose call inside the get_config_event failure branch, ensuring the
surrounding X11 display-server code compiles while preserving its existing
behavior.
In `@servers/rendering/renderer_rd/renderer_compositor_rd.cpp`:
- Around line 291-297: Update the non-scale branch in the boot-image rendering
logic to call DisplayServer::calculate_boot_image_rect(window_size, imgrect)
instead of manually adjusting screenrect2.position. Keep the existing p_scale
branch using OS::calculate_boot_screen_rect() unchanged.
---
Nitpick comments:
In `@platform/linuxbsd/x11/display_server_x11.cpp`:
- Around line 2717-2739: Update DisplayServerX11::pump_resize_events to replace
the single 16ms delay and drain with a bounded retry loop, similar to
WaylandThread::window_create() and using MAX_CONFIGURE_ROUNDTRIPS where
appropriate. Each iteration should wait one frame interval, synchronize, and
process pending ConfigureNotify events, while preserving the existing mutex
protection and avoiding unbounded waiting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffda4798-5725-4050-b41b-de1deab500b4
📒 Files selected for processing (19)
core/object/worker_thread_pool.cppcore/os/condition_variable.hdrivers/gles3/rasterizer_gles3.cppeditor/doc/editor_help.cppeditor/doc/editor_help.heditor/file_system/editor_file_system.cppeditor/inspector/editor_resource_preview.cppeditor/scene/2d/tiles/tiles_editor_plugin.cppeditor/scene/2d/tiles/tiles_editor_plugin.hmain/main.cppplatform/linuxbsd/wayland/display_server_wayland.cppplatform/linuxbsd/wayland/display_server_wayland.hplatform/linuxbsd/wayland/wayland_thread.cppplatform/linuxbsd/wayland/wayland_thread.hplatform/linuxbsd/x11/display_server_x11.cppplatform/linuxbsd/x11/display_server_x11.hservers/display_server.cppservers/display_server.hservers/rendering/renderer_rd/renderer_compositor_rd.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- core/object/worker_thread_pool.cpp
- editor/inspector/editor_resource_preview.cpp
- editor/scene/2d/tiles/tiles_editor_plugin.h
- editor/file_system/editor_file_system.cpp
- platform/linuxbsd/wayland/display_server_wayland.h
- platform/linuxbsd/wayland/display_server_wayland.cpp
- core/os/condition_variable.h
- servers/display_server.cpp
- editor/scene/2d/tiles/tiles_editor_plugin.cpp
- platform/linuxbsd/wayland/wayland_thread.h
- editor/doc/editor_help.cpp
| inline static SafeFlag _worker_thread_done = SafeFlag(false); | ||
| inline static SafeFlag _loader_thread_done = SafeFlag(false); | ||
| inline static bool _cleanup_in_progress = false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make _cleanup_in_progress thread-safe.
cleanup_doc() writes this flag while background worker/loader code uses cleanup state to stop or gate work. A plain bool creates a C++ data race; use SafeFlag (or equivalent atomic synchronization) consistently with the completion flags.
Proposed fix
- inline static bool _cleanup_in_progress = false;
+ inline static SafeFlag _cleanup_in_progress = SafeFlag(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline static SafeFlag _worker_thread_done = SafeFlag(false); | |
| inline static SafeFlag _loader_thread_done = SafeFlag(false); | |
| inline static bool _cleanup_in_progress = false; | |
| inline static SafeFlag _worker_thread_done = SafeFlag(false); | |
| inline static SafeFlag _loader_thread_done = SafeFlag(false); | |
| inline static SafeFlag _cleanup_in_progress = SafeFlag(false); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/doc/editor_help.h` around lines 207 - 209, Change _cleanup_in_progress
from a plain bool to SafeFlag, matching _worker_thread_done and
_loader_thread_done. Update its initialization and any reads or writes in
cleanup_doc() and related worker/loader paths to use the existing SafeFlag
interface consistently.
ce90c49 to
ea9244f
Compare
GeneralProtectionFault
force-pushed
the
splash_wayland
branch
from
ea9244f to
73e1fd1
Compare
JoltedJon
left a comment
JoltedJon
left a comment
There was a problem hiding this comment.
Tested on Hyprland and no more stretching was observed.
Hyprland lets users resize whatever it wants and if you resize during loading stretching will occur, the previous behavior was that the logo's position would offset in weird ways. The stretching in this instance seems fine to me because I have been told on other instances normally you cannot resize during loading.
5 participants
Fixes #1228
This refactors some of the startup/wayland/x11 code to fix the logo being scaled incorrectly with the startup window, or misplaced (Hyperland), both in compatibility mode and otherwise. In a nutshell, the cause is race conditions, one of the nastier ones being in show_window() (display_server_wayland.cpp), which calls window_create()--which stores the resolution in wayland_thread, but not in display_server_wayland (not until process_events() fires on the main thread).
So, the fix gets the window size right then and there.
I'm creating it as a draft because I think it warrants some review, and working on creating a diagram to illustrate the sequence of function calls. @JoltedJon has tested on Hyperland native, myself on a Hyperland VM (in that case, there were some unresolved issues with X11 as well), as well as Arch linux (Artix).
I will test on a Windows VM as well, but hopefully this only touches Linux given it's heavily on the Linux compositors/protocols.
Summary by CodeRabbit