Fix stretched/misplaced logo on startup & race conditions#1286
Fix stretched/misplaced logo on startup & race conditions#1286GeneralProtectionFault wants to merge 3 commits into
Conversation
WalkthroughAdds timed condition-variable waits and timeout-bounded shutdown loops for language, editor doc, filesystem, resource preview, and tile-preview worker threads; reorders main cleanup steps; introduces a compositor_sync() hook and calculate_boot_image_rect()/calculate_boot_screen_rect() helpers; updates Wayland/X11 window sizing and configure-event handling for boot splash correctness. ChangesThread shutdown synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Compositor sync and boot splash sizing
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant EditorHelp
participant WorkerThread
participant LoaderThread
participant MessageQueue
EditorHelp->>WorkerThread: start cache-load / generate_doc thread
EditorHelp->>LoaderThread: reset _loader_thread_done / _worker_thread_done
WorkerThread-->>EditorHelp: set _worker_thread_done on exit path
LoaderThread-->>EditorHelp: set _loader_thread_done on exit path
EditorHelp->>EditorHelp: cleanup_doc() sets _cleanup_in_progress
loop poll until done
EditorHelp->>MessageQueue: flush()
EditorHelp->>EditorHelp: check _loader_thread_done && _worker_thread_done
end
EditorHelp->>WorkerThread: wait_to_finish()
EditorHelp->>EditorHelp: delete doc
sequenceDiagram
participant Main
participant DisplayServerWayland
participant WaylandThread
participant Compositor
Main->>DisplayServerWayland: show_window()
DisplayServerWayland->>WaylandThread: window_create()
WaylandThread->>Compositor: roundtrip() (up to 10x)
Compositor-->>WaylandThread: configure event
WaylandThread-->>DisplayServerWayland: initial_configure_done = true
DisplayServerWayland->>WaylandThread: fetch WindowState / scale
DisplayServerWayland->>DisplayServerWayland: compute physical rect size, buffer_scale
DisplayServerWayland->>Compositor: wl_surface_set_buffer_scale (if changed)
Main->>DisplayServerWayland: calculate_boot_image_rect(window_size, imgrect)
DisplayServerWayland-->>Main: centered Rect2 for boot splash
Estimated code review effort: 4 (Complex) | ~70 minutes 🚥 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
| print_verbose(vformat("[display_server_wayland.cpp|show_window] Compositor physical size %s differs from requested %s, \ | ||
| syncing before EGL surface creation", | ||
| physical_size, wd.rect.size)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Line-continuation embeds tabs into the log string.
The \ at the end of the string literal splices the following indented line into the message, so the emitted text contains a run of tab characters mid-sentence. Use adjacent string-literal concatenation instead.
🧹 Proposed fix
- print_verbose(vformat("[display_server_wayland.cpp|show_window] Compositor physical size %s differs from requested %s, \
- syncing before EGL surface creation",
- physical_size, wd.rect.size));
+ print_verbose(vformat("[display_server_wayland.cpp|show_window] Compositor physical size %s differs from requested %s, "
+ "syncing before EGL surface creation",
+ physical_size, wd.rect.size));📝 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.
| print_verbose(vformat("[display_server_wayland.cpp|show_window] Compositor physical size %s differs from requested %s, \ | |
| syncing before EGL surface creation", | |
| physical_size, wd.rect.size)); | |
| print_verbose(vformat("[display_server_wayland.cpp|show_window] Compositor physical size %s differs from requested %s, " | |
| "syncing before EGL surface creation", | |
| physical_size, wd.rect.size)); |
🤖 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 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.
| // If no ConfigureNotify arrived yet and window is not maximized/fullscreen, | ||
| // poll briefly to let the WM assign the final size before the rendering | ||
| // context is used. | ||
| if (!get_config_event && !wd.maximized && !wd.fullscreen) { | ||
| for (int timeout = 0; timeout < 20; ++timeout) { | ||
| XSync(x11_display, False); | ||
| { | ||
| MutexLock mutex_lock(events_mutex); | ||
| XEvent config_event; | ||
| if (XCheckTypedWindowEvent(x11_display, wd.x11_window, ConfigureNotify, &config_event)) { | ||
| _window_changed(&config_event); | ||
| break; | ||
| } | ||
| } | ||
| OS::get_singleton()->delay_usec(5000); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether a background thread drains X11 events into polled_events.
rg -nP -C3 'polled_events' platform/linuxbsd/x11/display_server_x11.cpp | head -60
rg -nP '_poll_events|events_thread' platform/linuxbsd/x11/display_server_x11.cppRepository: Redot-Engine/redot-engine
Length of output: 1896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1918,1965p' platform/linuxbsd/x11/display_server_x11.cpp
printf '\n---\n'
sed -n '4588,4602p' platform/linuxbsd/x11/display_server_x11.cpp
printf '\n---\n'
sed -n '7388,7448p' platform/linuxbsd/x11/display_server_x11.cppRepository: Redot-Engine/redot-engine
Length of output: 4139
Also scan polled_events in this retry loop
The event thread can queue ConfigureNotify there before this XCheckTypedWindowEvent() call, so the fallback may miss the size update and wait the full 100 ms. Re-check polled_events inside the loop.
🤖 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 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.
4 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
New Features
Bug Fixes