Emscripten compilation and execution in browsers - #787
Conversation
|
|
ccummingsNV
left a comment
ccummingsNV
left a comment
There was a problem hiding this comment.
This is exciting stuff and looks mostly sane to me, but I'd need @skallweitNV to take a look as he understands the build process better than me.
I don't think we can really ship this without it being part of the testing suite either. Currently almost all tests run on all platforms. How do we want to approach that.
|
@j8asic thanks for the contribution. This is really cool to see! Let's first merge the slang-rhi PR and then get to this one after that. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds comprehensive Emscripten/WebAssembly support: platform detection macros, build presets and toolchain, many conditional compile paths and stubs for unsupported APIs, WASM example targets and HTML hosts, and runtime changes to use a window/surface swapchain for browser rendering. Changes
Sequence DiagramsequenceDiagram
participant App as Example App
participant Window as Browser Window / Canvas
participant Surface as Surface/Swapchain
participant Device as SGL Device
participant RHI as RHI Backend (WGPU)
participant GPU as GPU/WASM Runtime
App->>Window: create_window(canvasSelector)
Window-->>App: WindowHandle (canvasSelector)
App->>Surface: create_surface(WindowHandle)
Surface->>RHI: configure(preferred_format)
RHI-->>Surface: configured
loop Main loop
App->>Window: poll_events()
Window-->>App: events
alt surface configured
App->>Surface: acquire_next_image()
Surface->>RHI: get_presentable_image
RHI-->>Surface: image_view
App->>Device: create_command_buffer()
Device-->>App: command_buffer
App->>Device: record_render_commands(image_view)
Device->>RHI: submit(commands)
RHI->>GPU: execute
App->>Surface: present()
Surface->>RHI: present image
end
App->>App: emscripten_sleep(0)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
thanks @ccummingsNV @skallweitNV; bumping to the latest slang version and merging the slang-rhi PR, this is almost ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/sgl/device/cuda_utils.cpp (1)
3-408:⚠️ Potential issue | 🔴 CriticalUse a numeric feature guard
#if SGL_HAS_CUDAinstead of#ifdef.The macro
SGL_HAS_CUDAis defined as a numeric value (0 or 1) via CMake. The current code at line 3 uses#ifdef SGL_HAS_CUDAbefore including any config header, which is inconsistent with the rest of the codebase. All other files guard CUDA code with#if SGL_HAS_CUDAafter including the appropriate config headers. For numeric feature macros,#ifis the correct approach.Include
"sgl/core/config.h"first, then replace#ifdefwith#if:Proposed fix
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -#ifdef SGL_HAS_CUDA +#include "sgl/core/config.h" + +#if SGL_HAS_CUDA `#include` "cuda_utils.h"} // namespace sgl::cuda -#endif +#endif // SGL_HAS_CUDA🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/device/cuda_utils.cpp` around lines 3 - 408, At the top of this CUDA implementation (sgl::cuda namespace / functions like get_current_device_index and class Device), include the project config header by adding `#include` "sgl/core/config.h" before the feature guard, and replace the existing preprocessor check from "#ifdef SGL_HAS_CUDA" to "#if SGL_HAS_CUDA" (keeping the trailing `#endif` unchanged) so the numeric CMake-defined macro is tested correctly.src/sgl/core/memory_mapped_file.cpp (1)
201-241:⚠️ Potential issue | 🟠 MajorMove the Emscripten remap branch out of the Linux/macOS block.
The
#elif SGL_EMSCRIPTENbranch at line 221 is nested inside#elif SGL_LINUX || SGL_MACOS, making it unreachable. WhenSGL_EMSCRIPTENis defined, the outer condition evaluates to false (Emscripten is neither Linux nor macOS), so the entire block is skipped. The function then returnstruewithout settingm_mapped_dataorm_mapped_size. Move the Emscripten handling to a top-level#elif SGL_EMSCRIPTENbranch before the Linux/macOS block and returnfalse.Proposed fix
`#if` SGL_WINDOWS DWORD offsetLow = DWORD(offset & 0xFFFFFFFF); DWORD offsetHigh = DWORD(offset >> 32); // Create new mapping. m_mapped_data = ::MapViewOfFile(m_mapped_file, FILE_MAP_READ, offsetHigh, offsetLow, mapped_size); if (!m_mapped_data) m_mapped_size = 0; m_mapped_size = mapped_size; +#elif SGL_EMSCRIPTEN + // mmap is not available in the browser build. + m_mapped_data = nullptr; + m_mapped_size = 0; + return false; `#elif` SGL_LINUX || SGL_MACOS // Create new mapping. `#if` SGL_LINUX m_mapped_data = ::mmap64(NULL, mapped_size, PROT_READ, MAP_SHARED, m_file, offset); `#elif` SGL_MACOS m_mapped_data = ::mmap(NULL, mapped_size, PROT_READ, MAP_SHARED, m_file, offset); -#elif SGL_EMSCRIPTEN - m_mapped_data = MAP_FAILED; `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/core/memory_mapped_file.cpp` around lines 201 - 241, The Emscripten branch is wrongly nested inside the Linux/macOS conditional so when SGL_EMSCRIPTEN is defined m_mapped_data and m_mapped_size are never set; move the SGL_EMSCRIPTEN handling out to its own top-level `#elif` SGL_EMSCRIPTEN branch (parallel to the Windows and Linux/macOS branches) so it executes independently, set m_mapped_data = MAP_FAILED (or nullptr equivalent), set/clear m_mapped_size appropriately and return false on failure; update the branch locations around the existing MapViewOfFile (Windows) and mmap/madvise (Linux/macOS) code and ensure symbols m_mapped_data, m_mapped_size, SGL_EMSCRIPTEN, MAP_FAILED, mmap/mmap64, and ::madvise are used consistently.src/sgl/utils/renderdoc.cpp (1)
5-174:⚠️ Potential issue | 🔴 CriticalUse
#if !SGL_EMSCRIPTENinstead of#ifndef.The macro
SGL_EMSCRIPTENis defined as(SGL_COMPILER == SGL_COMPILER_EMSCRIPTEN)insrc/sgl/core/macros.hand evaluates to 0 or 1. Using#ifndef SGL_EMSCRIPTENalways evaluates to false (since the macro IS defined), making the entire RenderDoc implementation unreachable and preventing it from compiling on any build.Proposed fix
-#ifndef SGL_EMSCRIPTEN +#if !SGL_EMSCRIPTEN-#endif // SGL_EMSCRIPTEN +#endif // !SGL_EMSCRIPTENNote: The same incorrect
#ifndefpattern is used inrenderdoc.h(line 11),window.cpp(lines 28, 46, 571), andstring.cpp(line 122). They all need the same fix for consistency with the correct pattern already used inlmdb_cache.cpp(line 8).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/utils/renderdoc.cpp` around lines 5 - 174, The file uses an `#ifndef` SGL_EMSCRIPTEN guard which is wrong because SGL_EMSCRIPTEN is defined as 0/1; change the top-level preprocessor guard to `#if` !SGL_EMSCRIPTEN (and similarly update the matching endif comment if present) so the renderdoc::API class and its functions (API, API::start_frame_capture, API::end_frame_capture, is_available, start_frame_capture, end_frame_capture, is_frame_capturing) are compiled when not targeting Emscripten; apply the same fix to the other affected translation units that use the same pattern (renderdoc.h, window.cpp, string.cpp) for consistency.src/sgl/core/file_system_watcher.cpp (1)
77-148:⚠️ Potential issue | 🟡 MinorRemove unnecessary directory scan on Emscripten.
The
state->filesmember is populated on Emscripten (line 146-148) but never consumed, since the polling thread does not compile there:`#if` !SGL_LINUX state->files = get_directory_files(state->desc.directory); `#endif`On Emscripten, the struct member exists (guarded only by
#if !SGL_LINUX), but the thread that reads it is guarded by#if !SGL_LINUX && !SGL_EMSCRIPTEN(line 277). This wastes work scanning the directory on a platform where the result is unused. Align the guard:🔧 Suggested change
-#if !SGL_LINUX +#if !SGL_LINUX && !SGL_EMSCRIPTEN state->files = get_directory_files(state->desc.directory); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/core/file_system_watcher.cpp` around lines 77 - 148, The directory scan in FileSystemWatcher::add_watch currently populates state->files via get_directory_files even on Emscripten where the polling thread (m_thread/thread_func) is not compiled; change the guard around that assignment so it excludes Emscripten (i.e., only run state->files = get_directory_files(...) when not SGL_LINUX and not SGL_EMSCRIPTEN) to avoid doing unnecessary work on Emscripten platforms, keeping all other logic (id generation, state initialization, and Linux inotify handling) unchanged.
🧹 Nitpick comments (14)
examples/simple_compute/simple_compute.cpp (1)
15-19: Use the project platform macro and snake_case local constant.This works, but while adding the Emscripten branch, consider using
SGL_EMSCRIPTENconsistently and renaming the local constant toexample_dir. As per coding guidelines, C++ functions and variables use snake_case.♻️ Optional cleanup
-#ifdef __EMSCRIPTEN__ -static const std::filesystem::path EXAMPLE_DIR("."); +#if SGL_EMSCRIPTEN +static const std::filesystem::path example_dir("."); `#else` -static const std::filesystem::path EXAMPLE_DIR(SGL_EXAMPLE_DIR); +static const std::filesystem::path example_dir(SGL_EXAMPLE_DIR); `#endif` ... - .compiler_options = {.include_paths = {EXAMPLE_DIR}}, + .compiler_options = {.include_paths = {example_dir}},Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/simple_compute/simple_compute.cpp` around lines 15 - 19, Rename the platform-conditional local constant EXAMPLE_DIR to snake_case example_dir and switch the preprocessor check to use the project macro SGL_EMSCRIPTEN; specifically update the branch that currently tests __EMSCRIPTEN__ to use SGL_EMSCRIPTEN and replace occurrences of EXAMPLE_DIR with example_dir while preserving the same std::filesystem::path initialization from SGL_EXAMPLE_DIR or "." in the emscripten case so all uses (e.g., in this file and the other occurrence at lines ~30) follow the project's naming and macro conventions.src/sgl/core/platform.h (1)
33-34: Rename field to snake_case and add Doxygen documentation.
canvasSelectormust follow C++ naming conventions: use snake_case for variables and add Doxygen///comments for public fields. Update the initializer inwindow.cppand the consumer insurface.cpp.Refactor
`#elif` SGL_EMSCRIPTEN - const char* canvasSelector = nullptr; + /// CSS selector for the target WebGPU canvas. + const char* canvas_selector = nullptr; `#endif`Update call sites:
src/sgl/core/window.cpp:421:handle.canvasSelector→handle.canvas_selectorsrc/sgl/device/surface.cpp:27:window_handle.canvasSelector→window_handle.canvas_selector🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/core/platform.h` around lines 33 - 34, Rename the public field canvasSelector to snake_case canvas_selector and add a Doxygen triple-slash comment (///) describing the field in the struct/class guarded by SGL_EMSCRIPTEN; update its initializer in the window initialization code that sets handle.canvasSelector to use handle.canvas_selector, and update the consumer in the surface code that reads window_handle.canvasSelector to use window_handle.canvas_selector; ensure all references and includes compile after the rename and that the Doxygen comment describes the purpose (e.g., selector for the Emscripten canvas element).examples/wasm/CMakeLists.txt (1)
1-41: Consider a helper macro to deduplicate the two WASM example targets.Both targets repeat the same six operations (add_executable, cxx_std_20, link
sgl, preload.slang, defineSGL_EXAMPLE_DIR, post-build copy). A small helper keeps the list of examples easy to extend.Also note:
--preload-filealready bundles the.slanginto the Emscripten virtual filesystem, so the POST_BUILD copy next to the output duplicates the shader on disk — keep it if you need it for out-of-bundle access, otherwise it can be dropped.♻️ Suggested refactor
-# Render Pipeline Example -add_executable(render-pipeline-wasm - ../render_pipeline/render_pipeline.cpp -) - -target_compile_features(render-pipeline-wasm PRIVATE cxx_std_20) -target_link_libraries(render-pipeline-wasm PRIVATE sgl) -target_link_options(render-pipeline-wasm PRIVATE - "SHELL:--preload-file ${CMAKE_CURRENT_SOURCE_DIR}/../render_pipeline/render_pipeline.slang@render_pipeline.slang" -) -target_compile_definitions(render-pipeline-wasm PRIVATE SGL_EXAMPLE_DIR=".") - -add_custom_command(TARGET render-pipeline-wasm POST_BUILD - COMMAND ${CMAKE_COMMAND} -E copy - ${CMAKE_CURRENT_SOURCE_DIR}/../render_pipeline/render_pipeline.slang - $<TARGET_FILE_DIR:render-pipeline-wasm>/render_pipeline.slang - COMMAND ${CMAKE_COMMAND} -E copy - ${CMAKE_CURRENT_SOURCE_DIR}/render_pipeline.html - $<TARGET_FILE_DIR:render-pipeline-wasm>/render_pipeline.html -) - -# Simple Compute Example -add_executable(simple-compute-wasm - ../simple_compute/simple_compute.cpp -) - -target_compile_features(simple-compute-wasm PRIVATE cxx_std_20) -target_link_libraries(simple-compute-wasm PRIVATE sgl) -target_link_options(simple-compute-wasm PRIVATE - "SHELL:--preload-file ${CMAKE_CURRENT_SOURCE_DIR}/../simple_compute/simple_compute.slang@simple_compute.slang" -) -target_compile_definitions(simple-compute-wasm PRIVATE SGL_EXAMPLE_DIR=".") - -add_custom_command(TARGET simple-compute-wasm POST_BUILD - COMMAND ${CMAKE_COMMAND} -E copy - ${CMAKE_CURRENT_SOURCE_DIR}/../simple_compute/simple_compute.slang - $<TARGET_FILE_DIR:simple-compute-wasm>/simple_compute.slang - COMMAND ${CMAKE_COMMAND} -E copy - ${CMAKE_CURRENT_SOURCE_DIR}/simple_compute.html - $<TARGET_FILE_DIR:simple-compute-wasm>/simple_compute.html -) +macro(sgl_add_wasm_example target src_dir name) + add_executable(${target} ${CMAKE_CURRENT_SOURCE_DIR}/../${src_dir}/${name}.cpp) + target_compile_features(${target} PRIVATE cxx_std_20) + target_link_libraries(${target} PRIVATE sgl) + target_link_options(${target} PRIVATE + "SHELL:--preload-file ${CMAKE_CURRENT_SOURCE_DIR}/../${src_dir}/${name}.slang@${name}.slang" + ) + target_compile_definitions(${target} PRIVATE SGL_EXAMPLE_DIR=".") + add_custom_command(TARGET ${target} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + ${CMAKE_CURRENT_SOURCE_DIR}/${name}.html + $<TARGET_FILE_DIR:${target}>/${name}.html + ) +endmacro() + +sgl_add_wasm_example(render-pipeline-wasm render_pipeline render_pipeline) +sgl_add_wasm_example(simple-compute-wasm simple_compute simple_compute)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/wasm/CMakeLists.txt` around lines 1 - 41, Both WASM example targets (render-pipeline-wasm and simple-compute-wasm) duplicate the same CMake steps; add a helper macro/function (e.g., add_wasm_example(NAME SOURCE SLANG_HTML_BASENAME)) that performs add_executable, target_compile_features(... cxx_std_20), target_link_libraries(... sgl), target_link_options to add the "SHELL:--preload-file ${CMAKE_CURRENT_SOURCE_DIR}/../<example>/<basename>.slang@<basename>.slang" entry, target_compile_definitions(SGL_EXAMPLE_DIR="."), and the add_custom_command POST_BUILD that copies the .slang and .html; update calls for render-pipeline-wasm and simple-compute-wasm to use this macro and optionally remove the POST_BUILD copy if you rely solely on --preload-file for the virtual FS.examples/render_pipeline/render_pipeline.cpp (2)
130-135: Destructor order withdevice->close().
~App()callsdevice->close()before the ref-counted members (pipeline,program,input_layout, buffers,surface) are destroyed. This works today becauseDevice::close()(seedevice.cpp:460-498) only tears down device-owned helpers (blitter, printer, fences, slang_session, cuda_device) and leaves therhi_devicealive until~Device. But it is a subtle ordering that will silently break ifclose()ever releasesm_rhi_deviceeagerly. Consider explicitly resetting the dependent refs first, e.g.:~App() { - if (device) - device->close(); + pipeline.reset(); + program.reset(); + input_layout.reset(); + index_buffer.reset(); + vertex_buffer.reset(); + surface.reset(); + if (device) + device->close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/render_pipeline/render_pipeline.cpp` around lines 130 - 135, The destructor ~App() currently calls device->close() before releasing ref-counted members (pipeline, program, input_layout, buffers, surface), which can break if Device::close() later releases m_rhi_device; update ~App() to first explicitly reset/release all dependent ref-counted members (call reset() or clear() on pipeline, program, input_layout, surface and any buffers/containers holding RHI refs) and only after those are cleared call device->close() (and then release the device itself), so the dependent objects are destroyed before the device teardown.
137-154: Preferemscripten_set_main_loop(or at least a non-zero sleep) for the browser loop.The current
while (!should_close()) { main_loop(); emscripten_sleep(0); }pattern works under Asyncify, but:
emscripten_sleep(0)yields to the JS event loop but does not align with the browser's animation frame, and will effectively run at ~100% CPU.- Using
emscripten_set_main_loop(..., 0, 1)(fps=0 → rAF) is the idiomatic browser integration, removes the need for Asyncify for the main loop itself, and integrates naturally withrequestAnimationFrame.At minimum, bump the sleep (e.g.,
emscripten_sleep(16)) to roughly match vsync.♻️ Sketch using emscripten_set_main_loop_arg
- { - App app; - - while (!app.window->should_close()) { - app.main_loop(); -#ifdef __EMSCRIPTEN__ - emscripten_sleep(0); -#endif - } - } + { + App app; +#ifdef __EMSCRIPTEN__ + emscripten_set_main_loop_arg( + [](void* arg) { static_cast<App*>(arg)->main_loop(); }, + &app, 0 /* fps -> rAF */, 1 /* simulate infinite loop */); +#else + while (!app.window->should_close()) + app.main_loop(); +#endif + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/render_pipeline/render_pipeline.cpp` around lines 137 - 154, The current main loop in main() spins with emscripten_sleep(0); replace the busy loop by using emscripten_set_main_loop_arg to run a callback that calls app.main_loop() and checks app.window->should_close(), calling emscripten_cancel_main_loop() to stop when needed; alternatively, if you must keep the loop, change emscripten_sleep(0) to a non-zero value like emscripten_sleep(16) to throttle CPU. Locate the main function and the App instance (symbols: main, App, app, app.main_loop(), app.window->should_close()) and implement the main-loop callback (used with emscripten_set_main_loop_arg) or the increased sleep to align with vsync.CMakePresets.json (1)
201-213: PreferCMAKE_EXE_LINKER_FLAGS_INITand deduplicate withtarget_link_options.Two small improvements to the new
emscriptenpreset:
CMAKE_CXX_FLAGS_INIT(already used here) is the_INITform, so the user can later add flags.CMAKE_EXE_LINKER_FLAGSon the next line is the non-_INITform, which is sticky in the cache and harder to override. For symmetry and override-friendliness, useCMAKE_EXE_LINKER_FLAGS_INIT.-sUSE_GLFW=3,-sALLOW_MEMORY_GROWTH, and-fwasm-exceptionsare also set insrc/sgl/CMakeLists.txtviatarget_link_options(sgl PUBLIC ...). Pick one source of truth — ideally the target-level options (PUBLIC propagation handles executables linked againstsgl) — and drop the duplicates from the preset to prevent drift.- Document
-include cstdlib: it's a non-obvious workaround (presumably for a missing declaration in an Emscripten header) and a short comment via a JSON "description" field or a commit-log note will help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakePresets.json` around lines 201 - 213, Update the emscripten preset to use CMAKE_EXE_LINKER_FLAGS_INIT (mirror CMAKE_CXX_FLAGS_INIT) so linker flags remain override-friendly, remove duplicate flags that are already set at target level (drop -sUSE_GLFW=3, -sALLOW_MEMORY_GROWTH, -fwasm-exceptions from the preset) and rely on the existing target_link_options(sgl PUBLIC ...) in src/sgl/CMakeLists.txt for those options, and add a short note documenting the non-obvious "-include cstdlib" workaround (e.g., in the preset "description" field or commit message) so its purpose is recorded for future maintainers.src/sgl/device/device.cpp (1)
73-93: Standardize WASM detection onSGL_EMSCRIPTENinstead ofSLANG_WASM.This file uses
SLANG_WASM(from external Slang headers) for downstream compiler skip and automatic device type selection, while the rest of the codebase standardizes onSGL_EMSCRIPTEN(defined insrc/sgl/core/macros.h). The inconsistency creates maintenance risk—SLANG_WASMmay not be reliably synchronized withSGL_EMSCRIPTEN, which is derived from__EMSCRIPTEN__. Replace both instances at lines 73 and 84 withSGL_EMSCRIPTENto align with the codebase standard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/device/device.cpp` around lines 73 - 93, Replace uses of SLANG_WASM in src/sgl/device/device.cpp with the project-standard SGL_EMSCRIPTEN: change the first preprocessor guard from `#if` !SLANG_WASM to `#if` !SGL_EMSCRIPTEN (around the setDownstreamCompilerPath loop) and change the automatic device-type branch from `#if` SLANG_WASM to `#if` SGL_EMSCRIPTEN (the block that sets m_desc.type = DeviceType::wgpu). Ensure SGL_EMSCRIPTEN is available in this translation unit (include the header that defines it, e.g., sgl/core/macros.h) if not already included.src/sgl/core/thread.h (2)
8-10: UseSGL_EMSCRIPTENfor consistency.Since
sgl/core/macros.his already included above, prefer the project macro over the compiler-predefined one to match the rest of the file (e.g., line 21) and the convention adopted in this PR.♻️ Proposed change
-#ifndef __EMSCRIPTEN__ +#ifndef SGL_EMSCRIPTEN `#include` <nanothread/nanothread.h> -#endif +#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/core/thread.h` around lines 8 - 10, Replace the compiler macro check __EMSCRIPTEN__ with the project macro SGL_EMSCRIPTEN in the conditional include at the top of thread.h so it matches the rest of the file and the project's convention; specifically change the `#ifndef/__EMSCRIPTEN__` guard surrounding the nanothread include to use SGL_EMSCRIPTEN (the file already includes sgl/core/macros.h above), keeping the include and surrounding preprocessor structure identical otherwise.
21-130: Hoistblocked_rangetemplate outside#ifdefto eliminate duplication.
blocked_range<Int>is defined identically at line 40 (Emscripten) and line 214 (nanothread). Since it has no dependency on nanothread, move it before the#ifdef SGL_EMSCRIPTENto reduce code duplication.Regarding the missing
nanothreadsymbols:task_query,task_time, andtask_time_relare not used anywhere in the codebase, so their absence from the Emscripten mock is not a practical concern. Thedo_async,TaskGroup, and other public APIs are properly mocked for Emscripten builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/core/thread.h` around lines 21 - 130, The duplicated blocked_range<Int> definition should be hoisted out of the SGL_EMSCRIPTEN conditional: move the blocked_range template (including its iterator, blocks(), begin()/end(), block_size() and private members) above the `#ifdef` SGL_EMSCRIPTEN so both Emscripten and non-Emscripten paths reuse it; leave the Emscripten mocks for do_async, parallel_for, parallel_for_async, TaskGroup, and task_* wrappers as-is, and do not add unused nanothread symbols (task_query, task_time, task_time_rel) to the Emscripten section since they aren’t referenced.external/CMakeLists.txt (4)
132-139: Redundant innerif(NOT SGL_LOCAL_SLANG).Line 132 is already the
elseif(NOT SGL_EMSCRIPTEN)branch ofif(SGL_LOCAL_SLANG), soSGL_LOCAL_SLANGis guaranteed to be false here — the inner conditional on line 133 is always true.♻️ Proposed change
elseif(NOT SGL_EMSCRIPTEN) - if(NOT SGL_LOCAL_SLANG) - sgl_download_package(slang ${SLANG_URL}) - endif() - + sgl_download_package(slang ${SLANG_URL}) set(SLANG_DIR ${slang_SOURCE_DIR}) set(SLANG_INCLUDE_DIR ${slang_SOURCE_DIR}/include) endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@external/CMakeLists.txt` around lines 132 - 139, The inner conditional if(NOT SGL_LOCAL_SLANG) inside the elseif(NOT SGL_EMSCRIPTEN) branch is redundant because SGL_LOCAL_SLANG is already false in this branch; remove that inner if/endif and unindent its body so sgl_download_package(slang ${SLANG_URL}) and the subsequent set(...) calls (SLANG_DIR, SLANG_INCLUDE_DIR) execute directly within the elseif(NOT SGL_EMSCRIPTEN) block.
37-42:-include stdlib.hasINTERFACEonfmtleaks into every consumer.Propagating
-include stdlib.hviaINTERFACEforces this flag onto every target that linksfmt::fmt(which, persrc/sgl/CMakeLists.txt:367-395, is most of SGL and all examples). If the goal is only to fixfmt's own TUs,PRIVATEis sufficient. If theINTERFACEis required becausefmt's headers referencemalloc/freefrom templates, a brief comment explaining that would be helpful; otherwise consider dropping theINTERFACEline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@external/CMakeLists.txt` around lines 37 - 42, The INTERFACE -include stdlib.h flag on target_compile_options(fmt ...) is leaking into every consumer; change the second target_compile_options(fmt INTERFACE "-include" "stdlib.h") to be removed so only the PRIVATE -include is applied (i.e., keep target_compile_options(fmt PRIVATE "-include" "stdlib.h") only) unless you actually need consumers to inherit the include—if inheritance is required, replace the INTERFACE usage with a clear comment explaining why fmt headers require malloc/free visibility and document that this flag must propagate to consumers; ensure the code branch under SGL_EMSCRIPTEN only applies the PRIVATE compile option to fmt.
281-296:FORCE-settingSLANG_RHI_FETCH_SLANG/SLANG_RHI_BUILD_FROM_SLANG_REPOoverrides user/CI configuration.Using
CACHE BOOL "" FORCEhere unconditionally overwrites whatever the user passed on the command line or any parent-scope configuration, every time CMake is re-run. For the non-Emscripten path (line 294) this is a behavior change from the previous state where slang-rhi's own defaults applied.If the goal is only to set defaults, prefer plain
set(... )(non-cache) to propagate to the subdirectory, or drop theFORCEso users can override. If the goal really is a hard override for correctness, a short comment explaining why would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@external/CMakeLists.txt` around lines 281 - 296, The CMake snippet unconditionally forces SLANG_RHI_FETCH_SLANG and SLANG_RHI_BUILD_FROM_SLANG_REPO into the cache (using CACHE BOOL "" FORCE), which overrides user/CI configuration; change the two FORCE cache sets so they don't clobber user values: in the SGL_EMSCRIPTEN branch prefer non-cache set(SLANG_RHI_FETCH_SLANG OFF) / set(SLANG_RHI_BUILD_FROM_SLANG_REPO ON) or use CACHE without FORCE to provide defaults users can override, and similarly remove FORCE from the else() set(SLANG_RHI_FETCH_SLANG OFF CACHE BOOL "" ) or replace with a plain set(...) so the subproject/defaults remain overridable; if you truly need to hard-override, add a brief comment explaining why and keep FORCE intentionally.
142-146: Unreachable early return.
sgl_add_library_slang()is only invoked whenNOT SGL_EMSCRIPTEN OR SGL_LOCAL_SLANG(line 261), so theSGL_EMSCRIPTEN AND NOT SGL_LOCAL_SLANGguard here can never fire. Either drop the early return or drop the outer gate — keeping both makes the control flow harder to follow and hides the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@external/CMakeLists.txt` around lines 142 - 146, The early return inside function sgl_add_library_slang checks SGL_EMSCRIPTEN AND NOT SGL_LOCAL_SLANG but is unreachable because callers only invoke sgl_add_library_slang when that condition is already false; remove the redundant guard: delete the if(SGL_EMSCRIPTEN AND NOT SGL_LOCAL_SLANG) ... return() ... endif() block from sgl_add_library_slang so control flow is governed by the caller, or alternatively remove the external caller-side gate and keep the function-level guard — choose one consistent location for the conditional and remove the other to avoid duplicated/hidden logic.src/sgl/core/macros.h (1)
54-55: UseSGL_EMSCRIPTENvalue check rather thandefined().
SGL_EMSCRIPTENis defined unconditionally above (line 35) as an expression that evaluates to 0 or 1, sodefined(SGL_EMSCRIPTEN)is always true. It happens to work here because the preceding#if/#elifbranches filter out x86_64/arm64 first, but the intent is clearly a value check. For consistency with the rest of the file (e.g.,SGL_CLANG || SGL_GCCat line 119), prefer:♻️ Proposed change
-#elif defined(SGL_EMSCRIPTEN) +#elif SGL_EMSCRIPTEN `#define` SGL_ARCH SGL_ARCH_WASM🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/core/macros.h` around lines 54 - 55, The conditional uses defined(SGL_EMSCRIPTEN) but SGL_EMSCRIPTEN is defined earlier as 0/1, so replace the preprocessor check with a value check: change the branch that sets SGL_ARCH to SGL_ARCH_WASM to use `#elif` SGL_EMSCRIPTEN (or equivalent truthy evaluation) instead of defined(SGL_EMSCRIPTEN) so the check matches how other macros (e.g., SGL_CLANG || SGL_GCC) are evaluated and correctly honors the 0/1 definition of SGL_EMSCRIPTEN.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/render_pipeline/render_pipeline.slang`:
- Around line 11-23: The barycentric synthesis using uint id = vid % 3; inside
vertex_main (using vid and id to set output.barycentrics) only yields correct
results for sequential per-triangle vertex IDs (e.g., 0,1,2,0,1,2…) and will be
wrong for indexed draws, strips/fans or non-sequential IDs; update the
vertex_main implementation to either derive barycentrics from a safe
primitive-based source (e.g., SV_PrimitiveID combined with vertex index) when
available or, at minimum, add a clear comment above the uint id = vid % 3; line
explaining this limitation and warning against copy-paste reuse for
indexed/strip/fan draws so future readers know when to replace the hack.
In `@examples/wasm/simple_compute.html`:
- Around line 48-63: Add failure handlers so demo shows errors instead of
staying stuck on "Loading WASM module...": extend the existing Module object
(the same object that defines onRuntimeInitialized/print/printErr) with an
onAbort function that sets status.textContent to a descriptive failure message,
sets status.className to 'status error', and writes the error to output (similar
to printErr); also attach an onerror handler to the <script
src="simple-compute-wasm.js"> element (the script tag that loads the Emscripten
bundle) to set the same status and output text when the file fails to load.
Ensure you reuse the same status/output DOM variables and log the original error
message/URL for debugging.
In `@external/CMakeLists.txt`:
- Around line 313-322: The CMake fetch for slang-rhi uses a moving ref "GIT_TAG
main" which breaks reproducible builds; update the FetchContent_Declare call for
the slang-rhi dependency (the block guarded by SGL_EMSCRIPTEN that calls
FetchContent_Declare and FetchContent_MakeAvailable) to pin GIT_TAG to a
specific commit SHA or an explicit release tag instead of "main" (once the
companion slang-rhi PR/commit is finalized), and document the chosen SHA/tag in
a comment so future bumps are intentional.
In `@src/sgl/CMakeLists.txt`:
- Around line 417-428: The target_link_options block guarded by SGL_EMSCRIPTEN
for target "sgl" uses incompatible and duplicated Emscripten flags; update the
block to remove duplicates and fix the exceptions/Asyncify conflict by replacing
"-fwasm-exceptions" with "-fexceptions" (or alternatively set Asyncify mode to
JSPI via "-sASYNCIFY=2" if you need native wasm EH), and decide one
source-of-truth for flags (either keep them here on target_link_options PUBLIC
for target sgl or remove them from CMakePresets.json so they don't duplicate);
also ensure the PUBLIC propagation of "--no-entry" is intentional for downstream
consumers/examples that expect a no-entry wasm module.
In `@src/sgl/core/file_stream.cpp`:
- Around line 45-46: The Emscripten branch calls strerror(errnum) without
including the header that declares it; add the standard C++ header that provides
strerror (i.e., include <cstring>) at the top of the translation unit so all
branches (strerror, strerror_r, strerror_s) have the proper declaration; update
file_stream.cpp to add the <cstring> include near the other standard headers to
avoid relying on transitive includes.
In `@src/sgl/core/memory_mapped_file.h`:
- Around line 101-102: The Emscripten build path now uses the POSIX-like
FileHandle but memory_mapped_file.cpp references MAP_FAILED (e.g., in the remap
logic), and that macro is undefined because <sys/mman.h> is not included for the
Emscripten branch; fix by adding `#include` <sys/mman.h> to the Emscripten
conditional includes in memory_mapped_file.cpp so MAP_FAILED and related mmap
flags are defined (ensure the include is added alongside the existing
platform-specific includes for the Emscripten branch that uses FileHandle).
In `@src/sgl/core/object.h`:
- Around line 17-22: The typedef for Py_ssize_t_ under SGL_EMSCRIPTEN currently
forces int64_t which mismatches CPython's 32-bit Py_ssize_t on wasm32; change
the typedef to use intptr_t for Py_ssize_t_ when SGL_EMSCRIPTEN is defined (or
alternatively add a static_assert on sizeof(Py_ssize_t_) == sizeof(size_t) to
fail fast) so the width matches the platform; update the block around the
SGL_EMSCRIPTEN conditional that defines Py_ssize_t_ to use intptr_t (or add the
size static_assert) and keep the existing non-Emscripten branch and its
static_assert intact.
In `@src/sgl/core/platform_emscripten.cpp`:
- Around line 1-146: platform.cpp currently unconditionally includes
<GLFW/glfw3.h> and calls display_scale_factor() which uses GLFW, causing
Emscripten builds to fail; fix by making the GLFW dependency platform-gated:
either move GLFW-dependent logic (including display_scale_factor() and any GLFW
includes/uses) into an SGL non-Emscripten-specific source file, or wrap the GLFW
include and the display_scale_factor() implementation in `#if` !SGL_EMSCRIPTEN /
`#endif` guards so the header and GLFW calls are excluded for Emscripten while
preserving other functions like is_python_active() and format_stacktrace().
In `@src/sgl/core/string.cpp`:
- Around line 122-130: The conditional uses "#ifndef SGL_EMSCRIPTEN" which is
wrong because SGL_EMSCRIPTEN is always defined; change the preprocessor check to
use "#if !SGL_EMSCRIPTEN" so the TB branch (the fmt::format("{:.2f} TB", size /
1099511627776.0) return) is included on native builds; update the directive
surrounding the GB/TB returns in src/sgl/core/string.cpp (the block that
currently returns GB in the Emscripten case and TB for large sizes) to use `#if`
!SGL_EMSCRIPTEN / `#else` / `#endif` instead of `#ifndef`.
In `@src/sgl/core/window.cpp`:
- Around line 420-422: The code hardcodes "#canvas" into handle.canvasSelector
for SGL_EMSCRIPTEN; add an optional canvas_selector (or canvasSelector) field to
WindowDesc and make the Window creation/initialization (where
handle.canvasSelector is assigned) use WindowDesc.canvas_selector if present,
otherwise fall back to "#canvas" for backward compatibility; update any
Window/WindowDesc constructors or factory functions that build WindowHandle to
accept and propagate this new field so multiple WASM canvases can be targeted.
In `@src/sgl/device/surface.cpp`:
- Around line 26-27: The Surface(WindowHandle, ref<Device>) constructor passes
window_handle.canvasSelector directly into rhi::WindowHandle::fromWGPUCanvas
without checking for null; add a guard in the Surface constructor to detect a
null or empty canvasSelector on the incoming WindowHandle and substitute a safe
default (e.g. "#canvas") before calling rhi::WindowHandle::fromWGPUCanvas.
Locate the branch for SGL_EMSCRIPTEN in surface.cpp where
rhi::WindowHandle::fromWGPUCanvas(window_handle.canvasSelector) is used and wrap
it so that if window_handle.canvasSelector is null/empty you call fromWGPUCanvas
with the default selector instead.
---
Outside diff comments:
In `@src/sgl/core/file_system_watcher.cpp`:
- Around line 77-148: The directory scan in FileSystemWatcher::add_watch
currently populates state->files via get_directory_files even on Emscripten
where the polling thread (m_thread/thread_func) is not compiled; change the
guard around that assignment so it excludes Emscripten (i.e., only run
state->files = get_directory_files(...) when not SGL_LINUX and not
SGL_EMSCRIPTEN) to avoid doing unnecessary work on Emscripten platforms, keeping
all other logic (id generation, state initialization, and Linux inotify
handling) unchanged.
In `@src/sgl/core/memory_mapped_file.cpp`:
- Around line 201-241: The Emscripten branch is wrongly nested inside the
Linux/macOS conditional so when SGL_EMSCRIPTEN is defined m_mapped_data and
m_mapped_size are never set; move the SGL_EMSCRIPTEN handling out to its own
top-level `#elif` SGL_EMSCRIPTEN branch (parallel to the Windows and Linux/macOS
branches) so it executes independently, set m_mapped_data = MAP_FAILED (or
nullptr equivalent), set/clear m_mapped_size appropriately and return false on
failure; update the branch locations around the existing MapViewOfFile (Windows)
and mmap/madvise (Linux/macOS) code and ensure symbols m_mapped_data,
m_mapped_size, SGL_EMSCRIPTEN, MAP_FAILED, mmap/mmap64, and ::madvise are used
consistently.
In `@src/sgl/device/cuda_utils.cpp`:
- Around line 3-408: At the top of this CUDA implementation (sgl::cuda namespace
/ functions like get_current_device_index and class Device), include the project
config header by adding `#include` "sgl/core/config.h" before the feature guard,
and replace the existing preprocessor check from "#ifdef SGL_HAS_CUDA" to "#if
SGL_HAS_CUDA" (keeping the trailing `#endif` unchanged) so the numeric
CMake-defined macro is tested correctly.
In `@src/sgl/utils/renderdoc.cpp`:
- Around line 5-174: The file uses an `#ifndef` SGL_EMSCRIPTEN guard which is
wrong because SGL_EMSCRIPTEN is defined as 0/1; change the top-level
preprocessor guard to `#if` !SGL_EMSCRIPTEN (and similarly update the matching
endif comment if present) so the renderdoc::API class and its functions (API,
API::start_frame_capture, API::end_frame_capture, is_available,
start_frame_capture, end_frame_capture, is_frame_capturing) are compiled when
not targeting Emscripten; apply the same fix to the other affected translation
units that use the same pattern (renderdoc.h, window.cpp, string.cpp) for
consistency.
---
Nitpick comments:
In `@CMakePresets.json`:
- Around line 201-213: Update the emscripten preset to use
CMAKE_EXE_LINKER_FLAGS_INIT (mirror CMAKE_CXX_FLAGS_INIT) so linker flags remain
override-friendly, remove duplicate flags that are already set at target level
(drop -sUSE_GLFW=3, -sALLOW_MEMORY_GROWTH, -fwasm-exceptions from the preset)
and rely on the existing target_link_options(sgl PUBLIC ...) in
src/sgl/CMakeLists.txt for those options, and add a short note documenting the
non-obvious "-include cstdlib" workaround (e.g., in the preset "description"
field or commit message) so its purpose is recorded for future maintainers.
In `@examples/render_pipeline/render_pipeline.cpp`:
- Around line 130-135: The destructor ~App() currently calls device->close()
before releasing ref-counted members (pipeline, program, input_layout, buffers,
surface), which can break if Device::close() later releases m_rhi_device; update
~App() to first explicitly reset/release all dependent ref-counted members (call
reset() or clear() on pipeline, program, input_layout, surface and any
buffers/containers holding RHI refs) and only after those are cleared call
device->close() (and then release the device itself), so the dependent objects
are destroyed before the device teardown.
- Around line 137-154: The current main loop in main() spins with
emscripten_sleep(0); replace the busy loop by using emscripten_set_main_loop_arg
to run a callback that calls app.main_loop() and checks
app.window->should_close(), calling emscripten_cancel_main_loop() to stop when
needed; alternatively, if you must keep the loop, change emscripten_sleep(0) to
a non-zero value like emscripten_sleep(16) to throttle CPU. Locate the main
function and the App instance (symbols: main, App, app, app.main_loop(),
app.window->should_close()) and implement the main-loop callback (used with
emscripten_set_main_loop_arg) or the increased sleep to align with vsync.
In `@examples/simple_compute/simple_compute.cpp`:
- Around line 15-19: Rename the platform-conditional local constant EXAMPLE_DIR
to snake_case example_dir and switch the preprocessor check to use the project
macro SGL_EMSCRIPTEN; specifically update the branch that currently tests
__EMSCRIPTEN__ to use SGL_EMSCRIPTEN and replace occurrences of EXAMPLE_DIR with
example_dir while preserving the same std::filesystem::path initialization from
SGL_EXAMPLE_DIR or "." in the emscripten case so all uses (e.g., in this file
and the other occurrence at lines ~30) follow the project's naming and macro
conventions.
In `@examples/wasm/CMakeLists.txt`:
- Around line 1-41: Both WASM example targets (render-pipeline-wasm and
simple-compute-wasm) duplicate the same CMake steps; add a helper macro/function
(e.g., add_wasm_example(NAME SOURCE SLANG_HTML_BASENAME)) that performs
add_executable, target_compile_features(... cxx_std_20),
target_link_libraries(... sgl), target_link_options to add the
"SHELL:--preload-file
${CMAKE_CURRENT_SOURCE_DIR}/../<example>/<basename>.slang@<basename>.slang"
entry, target_compile_definitions(SGL_EXAMPLE_DIR="."), and the
add_custom_command POST_BUILD that copies the .slang and .html; update calls for
render-pipeline-wasm and simple-compute-wasm to use this macro and optionally
remove the POST_BUILD copy if you rely solely on --preload-file for the virtual
FS.
In `@external/CMakeLists.txt`:
- Around line 132-139: The inner conditional if(NOT SGL_LOCAL_SLANG) inside the
elseif(NOT SGL_EMSCRIPTEN) branch is redundant because SGL_LOCAL_SLANG is
already false in this branch; remove that inner if/endif and unindent its body
so sgl_download_package(slang ${SLANG_URL}) and the subsequent set(...) calls
(SLANG_DIR, SLANG_INCLUDE_DIR) execute directly within the elseif(NOT
SGL_EMSCRIPTEN) block.
- Around line 37-42: The INTERFACE -include stdlib.h flag on
target_compile_options(fmt ...) is leaking into every consumer; change the
second target_compile_options(fmt INTERFACE "-include" "stdlib.h") to be removed
so only the PRIVATE -include is applied (i.e., keep target_compile_options(fmt
PRIVATE "-include" "stdlib.h") only) unless you actually need consumers to
inherit the include—if inheritance is required, replace the INTERFACE usage with
a clear comment explaining why fmt headers require malloc/free visibility and
document that this flag must propagate to consumers; ensure the code branch
under SGL_EMSCRIPTEN only applies the PRIVATE compile option to fmt.
- Around line 281-296: The CMake snippet unconditionally forces
SLANG_RHI_FETCH_SLANG and SLANG_RHI_BUILD_FROM_SLANG_REPO into the cache (using
CACHE BOOL "" FORCE), which overrides user/CI configuration; change the two
FORCE cache sets so they don't clobber user values: in the SGL_EMSCRIPTEN branch
prefer non-cache set(SLANG_RHI_FETCH_SLANG OFF) /
set(SLANG_RHI_BUILD_FROM_SLANG_REPO ON) or use CACHE without FORCE to provide
defaults users can override, and similarly remove FORCE from the else()
set(SLANG_RHI_FETCH_SLANG OFF CACHE BOOL "" ) or replace with a plain set(...)
so the subproject/defaults remain overridable; if you truly need to
hard-override, add a brief comment explaining why and keep FORCE intentionally.
- Around line 142-146: The early return inside function sgl_add_library_slang
checks SGL_EMSCRIPTEN AND NOT SGL_LOCAL_SLANG but is unreachable because callers
only invoke sgl_add_library_slang when that condition is already false; remove
the redundant guard: delete the if(SGL_EMSCRIPTEN AND NOT SGL_LOCAL_SLANG) ...
return() ... endif() block from sgl_add_library_slang so control flow is
governed by the caller, or alternatively remove the external caller-side gate
and keep the function-level guard — choose one consistent location for the
conditional and remove the other to avoid duplicated/hidden logic.
In `@src/sgl/core/macros.h`:
- Around line 54-55: The conditional uses defined(SGL_EMSCRIPTEN) but
SGL_EMSCRIPTEN is defined earlier as 0/1, so replace the preprocessor check with
a value check: change the branch that sets SGL_ARCH to SGL_ARCH_WASM to use
`#elif` SGL_EMSCRIPTEN (or equivalent truthy evaluation) instead of
defined(SGL_EMSCRIPTEN) so the check matches how other macros (e.g., SGL_CLANG
|| SGL_GCC) are evaluated and correctly honors the 0/1 definition of
SGL_EMSCRIPTEN.
In `@src/sgl/core/platform.h`:
- Around line 33-34: Rename the public field canvasSelector to snake_case
canvas_selector and add a Doxygen triple-slash comment (///) describing the
field in the struct/class guarded by SGL_EMSCRIPTEN; update its initializer in
the window initialization code that sets handle.canvasSelector to use
handle.canvas_selector, and update the consumer in the surface code that reads
window_handle.canvasSelector to use window_handle.canvas_selector; ensure all
references and includes compile after the rename and that the Doxygen comment
describes the purpose (e.g., selector for the Emscripten canvas element).
In `@src/sgl/core/thread.h`:
- Around line 8-10: Replace the compiler macro check __EMSCRIPTEN__ with the
project macro SGL_EMSCRIPTEN in the conditional include at the top of thread.h
so it matches the rest of the file and the project's convention; specifically
change the `#ifndef/__EMSCRIPTEN__` guard surrounding the nanothread include to
use SGL_EMSCRIPTEN (the file already includes sgl/core/macros.h above), keeping
the include and surrounding preprocessor structure identical otherwise.
- Around line 21-130: The duplicated blocked_range<Int> definition should be
hoisted out of the SGL_EMSCRIPTEN conditional: move the blocked_range template
(including its iterator, blocks(), begin()/end(), block_size() and private
members) above the `#ifdef` SGL_EMSCRIPTEN so both Emscripten and non-Emscripten
paths reuse it; leave the Emscripten mocks for do_async, parallel_for,
parallel_for_async, TaskGroup, and task_* wrappers as-is, and do not add unused
nanothread symbols (task_query, task_time, task_time_rel) to the Emscripten
section since they aren’t referenced.
In `@src/sgl/device/device.cpp`:
- Around line 73-93: Replace uses of SLANG_WASM in src/sgl/device/device.cpp
with the project-standard SGL_EMSCRIPTEN: change the first preprocessor guard
from `#if` !SLANG_WASM to `#if` !SGL_EMSCRIPTEN (around the
setDownstreamCompilerPath loop) and change the automatic device-type branch from
`#if` SLANG_WASM to `#if` SGL_EMSCRIPTEN (the block that sets m_desc.type =
DeviceType::wgpu). Ensure SGL_EMSCRIPTEN is available in this translation unit
(include the header that defines it, e.g., sgl/core/macros.h) if not already
included.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3852d29b-9f63-475e-bc74-954024463cb8
📒 Files selected for processing (36)
.gitignoreCMakeLists.txtCMakePresets.jsonexamples/CMakeLists.txtexamples/render_pipeline/render_pipeline.cppexamples/render_pipeline/render_pipeline.slangexamples/simple_compute/simple_compute.cppexamples/wasm/CMakeLists.txtexamples/wasm/render_pipeline.htmlexamples/wasm/simple_compute.htmlexternal/CMakeLists.txtsrc/sgl/CMakeLists.txtsrc/sgl/core/file_stream.cppsrc/sgl/core/file_system_watcher.cppsrc/sgl/core/file_system_watcher.hsrc/sgl/core/lmdb_cache.cppsrc/sgl/core/macros.hsrc/sgl/core/memory_mapped_file.cppsrc/sgl/core/memory_mapped_file.hsrc/sgl/core/object.hsrc/sgl/core/platform.hsrc/sgl/core/platform_emscripten.cppsrc/sgl/core/string.cppsrc/sgl/core/thread.hsrc/sgl/core/window.cppsrc/sgl/device/cuda_utils.cppsrc/sgl/device/device.cppsrc/sgl/device/device.hsrc/sgl/device/native_handle_traits.hsrc/sgl/device/shader_cursor.cppsrc/sgl/device/shader_cursor.hsrc/sgl/device/shader_object.cppsrc/sgl/device/shader_object.hsrc/sgl/device/surface.cppsrc/sgl/utils/renderdoc.cppsrc/sgl/utils/renderdoc.h
| [shader("vertex")] | ||
| VertexOutput vertex_main(float2 pos: POSITION, uint vid: SV_VertexID) | ||
| { | ||
| VertexOutput output; | ||
| output.pos = float4(pos, 0.0, 1.0); | ||
|
|
||
| // Mimic SV_Barycentrics for backends that don't support it (e.g., WebGPU) | ||
| uint id = vid % 3; | ||
| output.barycentrics = float3(0, 0, 0); | ||
| output.barycentrics[id] = 1.0; | ||
|
|
||
| return output; | ||
| } |
There was a problem hiding this comment.
Barycentric synthesis via vid % 3 is only valid for this specific draw.
uint id = vid % 3; produces correct barycentrics only when the provided vertex IDs are sequential within each triangle (0,1,2,0,1,2,…). For indexed draws with arbitrary indices, or for triangle strips/fans, this will produce wrong results. That's fine for this one-triangle example, but please add a comment saying so to prevent copy-paste reuse, or derive it from SV_PrimitiveID/SV_VertexID only when safe.
As per coding guidelines (Slang shader code must use [shader("compute")] attribute for GPU entry points): note this applies to compute entry points — the [shader("vertex")] / [shader("fragment")] attributes here are correct for raster stages, no change needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/render_pipeline/render_pipeline.slang` around lines 11 - 23, The
barycentric synthesis using uint id = vid % 3; inside vertex_main (using vid and
id to set output.barycentrics) only yields correct results for sequential
per-triangle vertex IDs (e.g., 0,1,2,0,1,2…) and will be wrong for indexed
draws, strips/fans or non-sequential IDs; update the vertex_main implementation
to either derive barycentrics from a safe primitive-based source (e.g.,
SV_PrimitiveID combined with vertex index) when available or, at minimum, add a
clear comment above the uint id = vid % 3; line explaining this limitation and
warning against copy-paste reuse for indexed/strip/fan draws so future readers
know when to replace the hack.
| var Module = { | ||
| onRuntimeInitialized: function() { | ||
| status.textContent = '✓ Simple Compute WASM module loaded successfully!'; | ||
| status.className = 'status success'; | ||
| }, | ||
| print: function(text) { | ||
| console.log(text); | ||
| output.textContent += text + '\n'; | ||
| }, | ||
| printErr: function(text) { | ||
| console.error(text); | ||
| output.textContent += '[ERROR] ' + text + '\n'; | ||
| } | ||
| }; | ||
| </script> | ||
| <script src="simple-compute-wasm.js"></script> |
There was a problem hiding this comment.
Set the demo status on load/initialization failures.
Right now a missing simple-compute-wasm.js or Emscripten abort leaves the page stuck on “Loading WASM module...”. Add onAbort and a script onerror handler so browser demo failures are visible.
Proposed fix
var Module = {
onRuntimeInitialized: function() {
status.textContent = '✓ Simple Compute WASM module loaded successfully!';
status.className = 'status success';
},
+ onAbort: function(reason) {
+ status.textContent = '✗ Simple Compute WASM module failed to initialize';
+ status.className = 'status error';
+ output.textContent += '[ERROR] ' + reason + '\n';
+ },
print: function(text) {
console.log(text);
output.textContent += text + '\n';
},
printErr: function(text) {
console.error(text);
output.textContent += '[ERROR] ' + text + '\n';
}
};
</script>
- <script src="simple-compute-wasm.js"></script>
+ <script
+ src="simple-compute-wasm.js"
+ onerror="status.textContent='✗ Failed to load simple-compute-wasm.js'; status.className='status error';">
+ </script>📝 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.
| var Module = { | |
| onRuntimeInitialized: function() { | |
| status.textContent = '✓ Simple Compute WASM module loaded successfully!'; | |
| status.className = 'status success'; | |
| }, | |
| print: function(text) { | |
| console.log(text); | |
| output.textContent += text + '\n'; | |
| }, | |
| printErr: function(text) { | |
| console.error(text); | |
| output.textContent += '[ERROR] ' + text + '\n'; | |
| } | |
| }; | |
| </script> | |
| <script src="simple-compute-wasm.js"></script> | |
| var Module = { | |
| onRuntimeInitialized: function() { | |
| status.textContent = '✓ Simple Compute WASM module loaded successfully!'; | |
| status.className = 'status success'; | |
| }, | |
| onAbort: function(reason) { | |
| status.textContent = '✗ Simple Compute WASM module failed to initialize'; | |
| status.className = 'status error'; | |
| output.textContent += '[ERROR] ' + reason + '\n'; | |
| }, | |
| print: function(text) { | |
| console.log(text); | |
| output.textContent += text + '\n'; | |
| }, | |
| printErr: function(text) { | |
| console.error(text); | |
| output.textContent += '[ERROR] ' + text + '\n'; | |
| } | |
| }; | |
| </script> | |
| <script | |
| src="simple-compute-wasm.js" | |
| onerror="status.textContent='✗ Failed to load simple-compute-wasm.js'; status.className='status error';"> | |
| </script> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/wasm/simple_compute.html` around lines 48 - 63, Add failure handlers
so demo shows errors instead of staying stuck on "Loading WASM module...":
extend the existing Module object (the same object that defines
onRuntimeInitialized/print/printErr) with an onAbort function that sets
status.textContent to a descriptive failure message, sets status.className to
'status error', and writes the error to output (similar to printErr); also
attach an onerror handler to the <script src="simple-compute-wasm.js"> element
(the script tag that loads the Emscripten bundle) to set the same status and
output text when the file fails to load. Ensure you reuse the same status/output
DOM variables and log the original error message/URL for debugging.
| elseif(SGL_EMSCRIPTEN) | ||
| # Emscripten uses upstream slang-rhi directly here because the checked-in | ||
| # submodule may still be pinned to an older commit. | ||
| FetchContent_Declare( | ||
| slang-rhi | ||
| GIT_REPOSITORY https://github.com/shader-slang/slang-rhi.git | ||
| GIT_TAG main | ||
| GIT_SHALLOW TRUE | ||
| ) | ||
| FetchContent_MakeAvailable(slang-rhi) |
There was a problem hiding this comment.
GIT_TAG main breaks build reproducibility.
Fetching slang-rhi at main means every clean configure may pull a different commit, so two developers (or CI runs a day apart) can end up with silently different SGL behavior — and bisection becomes unreliable. Please pin to a specific commit SHA (or a tag) once the companion slang-rhi PR is merged, and bump it intentionally.
♻️ Suggested change
FetchContent_Declare(
slang-rhi
GIT_REPOSITORY https://github.com/shader-slang/slang-rhi.git
- GIT_TAG main
+ GIT_TAG <pinned-commit-sha>
GIT_SHALLOW TRUE
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@external/CMakeLists.txt` around lines 313 - 322, The CMake fetch for
slang-rhi uses a moving ref "GIT_TAG main" which breaks reproducible builds;
update the FetchContent_Declare call for the slang-rhi dependency (the block
guarded by SGL_EMSCRIPTEN that calls FetchContent_Declare and
FetchContent_MakeAvailable) to pin GIT_TAG to a specific commit SHA or an
explicit release tag instead of "main" (once the companion slang-rhi PR/commit
is finalized), and document the chosen SHA/tag in a comment so future bumps are
intentional.
| if(SGL_EMSCRIPTEN) | ||
| target_link_options(sgl | ||
| PUBLIC | ||
| "-sUSE_GLFW=3" | ||
| "--use-port=emdawnwebgpu" | ||
| "-fwasm-exceptions" | ||
| "-sASYNCIFY=1" | ||
| "-sALLOW_MEMORY_GROWTH=1" | ||
| "-sMAXIMUM_MEMORY=4GB" | ||
| "--no-entry" | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Emscripten support combining -fwasm-exceptions with -sASYNCIFY=1? Any required additional flags or caveats?
💡 Result:
No, Emscripten does not fully support combining -fwasm-exceptions with -sASYNCIFY=1. The compiler emits a warning: "ASYNCIFY=1 is not compatible with -fwasm-exceptions. Parts of the program that mix ASYNCIFY and exceptions will not compile." Builds may succeed but fail at runtime or during wasm-opt if code mixes async suspend points and exception handling (try-catch blocks), due to incomplete Binaryen support for transforming Wasm EH try constructs under Asyncify. Partial upstream support exists in Binaryen since 2023, but Emscripten warns due to reliability issues. Use -fexceptions (JS-based) with ASYNCIFY instead, or JSPI (-sASYNCIFY=2) with -fwasm-exceptions for better compatibility. No additional flags resolve the core incompatibility. As of Emscripten 5.0.6-git (2026), official docs do not list full support, and changelog lacks confirmation.
Citations:
- 1: Add partial support for -fwasm-exceptions in Asyncify (#5343) WebAssembly/binaryen#5475
- 2:
-sASYNCIFY=2does not work with-fexceptionsemscripten-core/emscripten#19672 - 3: Support Asyncify + Try-Catch [Can't build web-gphoto2 with -fwasm-exceptions instead of -fexceptions] WebAssembly/binaryen#4470
- 4: https://emscripten.org/docs/porting/asyncify.html
- 5: https://emscripten.org/docs/porting/exceptions.html
🏁 Script executed:
#!/bin/bash
# Find and inspect CMakePresets.json for Emscripten-related flags
find . -name "CMakePresets.json" -type fRepository: shader-slang/slangpy
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Check CMakePresets.json for Emscripten preset and linker flags
cat CMakePresets.json | head -200Repository: shader-slang/slangpy
Length of output: 6311
🏁 Script executed:
#!/bin/bash
# Search for Emscripten preset and its linker flags configuration
rg -A 30 '"name".*emscripten' CMakePresets.json -iRepository: shader-slang/slangpy
Length of output: 1996
Fix incompatible Emscripten link flags and consolidate duplicates.
Three issues with the Emscripten target_link_options block:
-
Critical:
-fwasm-exceptionsand-sASYNCIFY=1are incompatible. Emscripten emits a warning: "ASYNCIFY=1 is not compatible with -fwasm-exceptions." Code mixing async suspend points and exception handling will fail at runtime or during wasm-opt optimization. Replace-fwasm-exceptionswith-fexceptions(JS-based exceptions) to keep Asyncify, or switch to JSPI (-sASYNCIFY=2) if you need native wasm EH. -
-sUSE_GLFW=3,-fwasm-exceptions, and-sALLOW_MEMORY_GROWTHare already set inCMakePresets.json(CMAKE_EXE_LINKER_FLAGS). Duplicated link flags make the final command line noisy and allow drift. Keep one source of truth—either the target-level options here (PUBLIC propagates to all consumers) and remove overlap from the preset, or vice versa. -
--no-entryis correct for a STATIC library on Emscripten, but it will propagate PUBLICly to anything linkingsgl. Ensure all example targets expect a no-entry/wasm-module layout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/CMakeLists.txt` around lines 417 - 428, The target_link_options block
guarded by SGL_EMSCRIPTEN for target "sgl" uses incompatible and duplicated
Emscripten flags; update the block to remove duplicates and fix the
exceptions/Asyncify conflict by replacing "-fwasm-exceptions" with
"-fexceptions" (or alternatively set Asyncify mode to JSPI via "-sASYNCIFY=2" if
you need native wasm EH), and decide one source-of-truth for flags (either keep
them here on target_link_options PUBLIC for target sgl or remove them from
CMakePresets.json so they don't duplicate); also ensure the PUBLIC propagation
of "--no-entry" is intentional for downstream consumers/examples that expect a
no-entry wasm module.
| #elif SGL_EMSCRIPTEN | ||
| return strerror(errnum); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i '^file_stream\.cpp$' --exec sh -c '
printf "\n== %s ==\n" "$1"
sed -n "1,55p" "$1"
' sh {}
rg -n -C2 '\b(strerror|strerror_r|errno)\b' --iglob 'file_stream.cpp'Repository: shader-slang/slangpy
Length of output: 4225
Add the header declaring strerror().
The Emscripten branch (line 46) calls strerror() without a direct include for the header that declares it. All platform branches use string error functions (strerror(), strerror_r(), strerror_s()), which require <cstring> to be declared. Relying on transitive includes is fragile and may break with toolchain changes.
🔧 Proposed include fix
`#include` "sgl/core/file_stream.h"
`#include` "sgl/core/error.h"
+#include <cstring>
`#include` <fstream>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/core/file_stream.cpp` around lines 45 - 46, The Emscripten branch
calls strerror(errnum) without including the header that declares it; add the
standard C++ header that provides strerror (i.e., include <cstring>) at the top
of the translation unit so all branches (strerror, strerror_r, strerror_s) have
the proper declaration; update file_stream.cpp to add the <cstring> include near
the other standard headers to avoid relying on transitive includes.
| #if SGL_EMSCRIPTEN | ||
| typedef int64_t Py_ssize_t_; | ||
| #else | ||
| typedef intptr_t Py_ssize_t_; | ||
| static_assert(sizeof(Py_ssize_t_) == sizeof(size_t)); | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On wasm32 Emscripten builds (including pyodide), is CPython's Py_ssize_t 32-bit or 64-bit?
💡 Result:
On wasm32 Emscripten builds (including Pyodide), CPython's Py_ssize_t is 32-bit.
Citations:
- 1: gh-84461: Fix ctypes and test_ctypes on Emscripten python/cpython#94142
- 2: https://github.com/python/cpython/blob/main/Platforms/emscripten/README.md
- 3: https://stackoverflow.com/questions/75559641/memory-limits-in-pyodide
- 4: size_t signature change breaks 32-bit only code emscripten-core/emscripten#6973
🏁 Script executed:
#!/bin/bash
# Search for SGL_EMSCRIPTEN usage and context
rg "SGL_EMSCRIPTEN" --type cpp --type h -B 3 -A 3
# Search for any wasm64 or build documentation
rg "wasm64|MEMORY64" --type cpp --type h --type py --type txt
# Check if there are any comments about Py_ssize_t in the codebase
rg "Py_ssize_t" --type cpp --type h -B 2 -A 2Repository: shader-slang/slangpy
Length of output: 20259
Use intptr_t or add static_assert to catch Emscripten size mismatch.
On wasm32 (the standard Emscripten target, incl. pyodide), CPython's Py_ssize_t is 32-bit. Typedefing Py_ssize_t_ as int64_t here creates a width mismatch that will break Python bindings if ever wired on Emscripten. The codebase shows no wasm64 support, making int64_t appear incorrect. Either change it to intptr_t (which will be 32-bit and match CPython on wasm32) or add the missing size assertion to expose the mismatch at compile time.
Suggested fix
`#if` SGL_EMSCRIPTEN
-typedef int64_t Py_ssize_t_;
+typedef intptr_t Py_ssize_t_;
+static_assert(sizeof(Py_ssize_t_) == sizeof(size_t));
`#else`
typedef intptr_t Py_ssize_t_;
static_assert(sizeof(Py_ssize_t_) == sizeof(size_t));
`#endif`📝 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.
| #if SGL_EMSCRIPTEN | |
| typedef int64_t Py_ssize_t_; | |
| #else | |
| typedef intptr_t Py_ssize_t_; | |
| static_assert(sizeof(Py_ssize_t_) == sizeof(size_t)); | |
| #endif | |
| `#if` SGL_EMSCRIPTEN | |
| typedef intptr_t Py_ssize_t_; | |
| static_assert(sizeof(Py_ssize_t_) == sizeof(size_t)); | |
| `#else` | |
| typedef intptr_t Py_ssize_t_; | |
| static_assert(sizeof(Py_ssize_t_) == sizeof(size_t)); | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/core/object.h` around lines 17 - 22, The typedef for Py_ssize_t_
under SGL_EMSCRIPTEN currently forces int64_t which mismatches CPython's 32-bit
Py_ssize_t on wasm32; change the typedef to use intptr_t for Py_ssize_t_ when
SGL_EMSCRIPTEN is defined (or alternatively add a static_assert on
sizeof(Py_ssize_t_) == sizeof(size_t) to fail fast) so the width matches the
platform; update the block around the SGL_EMSCRIPTEN conditional that defines
Py_ssize_t_ to use intptr_t (or add the size static_assert) and keep the
existing non-Emscripten branch and its static_assert intact.
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| #include "platform.h" | ||
|
|
||
| #if SGL_EMSCRIPTEN | ||
|
|
||
| #include "sgl/core/error.h" | ||
| #include <iostream> | ||
|
|
||
| namespace sgl::platform { | ||
|
|
||
| void static_init() { } | ||
|
|
||
| void static_shutdown() { } | ||
|
|
||
| void set_window_icon(WindowHandle handle, const std::filesystem::path& path) | ||
| { | ||
| SGL_UNUSED(handle); | ||
| SGL_UNUSED(path); | ||
| } | ||
|
|
||
| void set_keyboard_interrupt_handler(std::function<void()> handler) | ||
| { | ||
| SGL_UNUSED(handler); | ||
| } | ||
|
|
||
| std::optional<std::filesystem::path> open_file_dialog(std::span<const FileDialogFilter> filters) | ||
| { | ||
| SGL_UNUSED(filters); | ||
| return {}; | ||
| } | ||
|
|
||
| std::optional<std::filesystem::path> save_file_dialog(std::span<const FileDialogFilter> filters) | ||
| { | ||
| SGL_UNUSED(filters); | ||
| return {}; | ||
| } | ||
|
|
||
| std::optional<std::filesystem::path> choose_folder_dialog() | ||
| { | ||
| return {}; | ||
| } | ||
|
|
||
| bool create_junction(const std::filesystem::path& link, const std::filesystem::path& target) | ||
| { | ||
| SGL_UNUSED(link); | ||
| SGL_UNUSED(target); | ||
| return false; | ||
| } | ||
|
|
||
| bool delete_junction(const std::filesystem::path& link) | ||
| { | ||
| SGL_UNUSED(link); | ||
| return false; | ||
| } | ||
|
|
||
| const std::filesystem::path& executable_path() | ||
| { | ||
| static std::filesystem::path path("/"); | ||
| return path; | ||
| } | ||
|
|
||
| const std::filesystem::path& app_data_directory() | ||
| { | ||
| static std::filesystem::path path("/"); | ||
| return path; | ||
| } | ||
|
|
||
| const std::filesystem::path& home_directory() | ||
| { | ||
| static std::filesystem::path path("/"); | ||
| return path; | ||
| } | ||
|
|
||
| const std::filesystem::path& runtime_directory() | ||
| { | ||
| static std::filesystem::path path("/"); | ||
| return path; | ||
| } | ||
|
|
||
| std::optional<std::string> get_environment_variable(const char* name) | ||
| { | ||
| SGL_UNUSED(name); | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| ProcessID current_process_id() | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| size_t page_size() | ||
| { | ||
| return 4096; | ||
| } | ||
|
|
||
| MemoryStats memory_stats() | ||
| { | ||
| return {0, 0}; | ||
| } | ||
|
|
||
| SharedLibraryHandle load_shared_library(const std::filesystem::path& path) | ||
| { | ||
| SGL_UNUSED(path); | ||
| return nullptr; | ||
| } | ||
|
|
||
| void release_shared_library(SharedLibraryHandle library) | ||
| { | ||
| SGL_UNUSED(library); | ||
| } | ||
|
|
||
| void* get_proc_address(SharedLibraryHandle library, const char* proc_name) | ||
| { | ||
| SGL_UNUSED(library); | ||
| SGL_UNUSED(proc_name); | ||
| return nullptr; | ||
| } | ||
|
|
||
| bool is_debugger_present() | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| void debug_break() { } | ||
|
|
||
| void print_to_debug_window(const char* str) | ||
| { | ||
| std::cerr << str; | ||
| } | ||
|
|
||
| StackTrace backtrace(size_t skip_frames) | ||
| { | ||
| SGL_UNUSED(skip_frames); | ||
| return {}; | ||
| } | ||
|
|
||
| ResolvedStackTrace resolve_stacktrace(std::span<const StackFrame> trace) | ||
| { | ||
| SGL_UNUSED(trace); | ||
| return {}; | ||
| } | ||
|
|
||
| } // namespace sgl::platform | ||
|
|
||
| #endif // SGL_EMSCRIPTEN |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Confirm platform.cpp participates in the Emscripten build (no top-level `#if` !SGL_EMSCRIPTEN wrapping everything).
fd -t f 'platform(_.+)?\.cpp' src/sgl/core
echo '--- top of platform.cpp ---'
fd -t f '^platform\.cpp$' src/sgl/core --exec head -n 40 {}
echo '--- CMakeLists for src/sgl ---'
rg -n 'platform(_emscripten)?\.cpp' src/sgl/CMakeLists.txt
# 2. Enumerate public sgl::platform function declarations in platform.h and compare against what's implemented here.
rg -nP '^\s*(SGL_API\s+)?[\w:<>,\s\*&]+\s+\w+\s*\([^;{]*\)\s*;' src/sgl/core/platform.h | rg -v '^\s*//'
# 3. Spot-check references used from error.cpp and logger.cpp.
rg -nP 'platform::(is_python_active|format_stacktrace|backtrace|resolve_stacktrace|get_environment_variable)\b' --type=cppRepository: shader-slang/slangpy
Length of output: 2535
🏁 Script executed:
#!/bin/bash
# Find where format_stacktrace is defined
echo "=== Searching for format_stacktrace definition ==="
rg -n 'format_stacktrace\s*\(' src/sgl/core --type=cpp -A 2 | head -30
# Check if platform.cpp has conditional guards for platform-specific code
echo -e "\n=== Checking for guards in platform.cpp (search for GLFW or `#if`) ==="
rg -n '(`#if`|#ifdef|GLFW)' src/sgl/core/platform.cpp | head -20
# Check CMakeLists.txt for full context on how platform files are conditionally included
echo -e "\n=== CMakeLists.txt context around platform files ==="
rg -n -B 5 -A 5 'platform_emscripten\.cpp' src/sgl/CMakeLists.txtRepository: shader-slang/slangpy
Length of output: 2237
🏁 Script executed:
#!/bin/bash
# Check if CMakeLists uses if() blocks around source file selection
echo "=== Full CMakeLists structure (lines 40-80) ==="
sed -n '40,80p' src/sgl/CMakeLists.txt
# Also check for conditional variables or platform detection
echo -e "\n=== Search for conditional file list (if/endif patterns) ==="
rg -n 'if\s*\(|endif\s*\(' src/sgl/CMakeLists.txt | head -20
# Look for Emscripten or platform-specific conditionals
echo -e "\n=== Search for EMSCRIPTEN or platform checks ==="
rg -n 'EMSCRIPTEN|MINGW|MSVC|APPLE' src/sgl/CMakeLists.txtRepository: shader-slang/slangpy
Length of output: 2332
🏁 Script executed:
#!/bin/bash
# Check entire platform.cpp for `#if` guards and structure
echo "=== platform.cpp with line numbers (first 100 lines) ==="
head -n 100 src/sgl/core/platform.cpp | cat -n
echo -e "\n=== Search for `#if/`#else/#endif patterns in platform.cpp ==="
rg -n '#if|#else|#endif' src/sgl/core/platform.cppRepository: shader-slang/slangpy
Length of output: 3249
🏁 Script executed:
#!/bin/bash
# Get full platform.cpp line count and check for actual `#if` guards
echo "=== Total lines in platform.cpp ==="
wc -l src/sgl/core/platform.cpp
echo -e "\n=== Check for any `#if` guard patterns anywhere in file ==="
grep -n '#if' src/sgl/core/platform.cpp
# Also check if platform.h might have guards around GLFW
echo -e "\n=== Check platform.h for GLFW guards or conditional ==="
grep -n 'GLFW\|#if' src/sgl/core/platform.h | head -20
# Look at what functions are actually in platform_emscripten vs platform.cpp
echo -e "\n=== Functions defined in platform_emscripten.cpp ==="
rg -n '^\w+\s+\w+\s*\(' src/sgl/core/platform_emscripten.cpp | grep -v '//'
echo -e "\n=== Functions defined in platform.cpp (sampling) ==="
rg -n '^\w+\s+\w+\s*\(' src/sgl/core/platform.cpp | head -15Repository: shader-slang/slangpy
Length of output: 1472
🏁 Script executed:
#!/bin/bash
# Check if platform.cpp itself is conditionally included in CMakeLists
echo "=== Full source list context in CMakeLists (lines 30-90) ==="
sed -n '30,90p' src/sgl/CMakeLists.txt
# Check which functions from platform.cpp are actually called from Emscripten code
echo -e "\n=== Look for calls to functions defined in platform.cpp ==="
rg -n 'is_python_active|set_python_active|display_scale_factor|format_stacktrace' src/sgl/core --type=cpp
# Summary: verify what gets compiled
echo -e "\n=== Verify: is platform.cpp inside or outside the source list? ==="
sed -n '35,65p' src/sgl/CMakeLists.txt | grep -E 'platform|add_library'Repository: shader-slang/slangpy
Length of output: 3479
platform.cpp unconditionally includes GLFW, which will cause compilation to fail on Emscripten.
platform.cpp is always compiled (it's not conditionally excluded in CMakeLists.txt), but it unconditionally includes <GLFW/glfw3.h> (line 7) with unguarded usage in display_scale_factor() (lines 30–32). Since GLFW is excluded from the Emscripten build (CMakeLists.txt line 379: $<$<NOT:$<BOOL:${SGL_EMSCRIPTEN}>>:glfw>), the compilation will fail with "glfw3.h: No such file or directory" when targeting WebAssembly.
Functions like is_python_active() and format_stacktrace() are correctly defined in platform.cpp, so they are available; however, the GLFW dependency must be resolved. Either move the GLFW-dependent functions to a platform-specific file or wrap the GLFW #include and usage in conditional compilation guards (e.g., #if !SGL_EMSCRIPTEN).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/core/platform_emscripten.cpp` around lines 1 - 146, platform.cpp
currently unconditionally includes <GLFW/glfw3.h> and calls
display_scale_factor() which uses GLFW, causing Emscripten builds to fail; fix
by making the GLFW dependency platform-gated: either move GLFW-dependent logic
(including display_scale_factor() and any GLFW includes/uses) into an SGL
non-Emscripten-specific source file, or wrap the GLFW include and the
display_scale_factor() implementation in `#if` !SGL_EMSCRIPTEN / `#endif` guards so
the header and GLFW calls are excluded for Emscripten while preserving other
functions like is_python_active() and format_stacktrace().
| #ifndef SGL_EMSCRIPTEN | ||
| else if (size < 1099511627776ull) | ||
| return fmt::format("{:.2f} GB", size / 1073741824.0); | ||
| else | ||
| return fmt::format("{:.2f} TB", size / 1099511627776.0); | ||
| #else | ||
| else | ||
| return fmt::format("{:.2f} GB", size / 1073741824.0); | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm SGL_EMSCRIPTEN is a numeric macro and locate `#ifndef` uses.
# Expected: Numeric 0/1 definition; any `#ifndef` SGL_EMSCRIPTEN sites should be converted to `#if` !SGL_EMSCRIPTEN.
rg -n -C3 '#\s*(define|cmakedefine01)\s+SGL_EMSCRIPTEN|#\s*ifndef\s+SGL_EMSCRIPTEN|#\s*if\s+!SGL_EMSCRIPTEN'Repository: shader-slang/slangpy
Length of output: 3236
Use #if !SGL_EMSCRIPTEN instead of #ifndef SGL_EMSCRIPTEN.
Since SGL_EMSCRIPTEN is always defined (as a boolean expression in macros.h), the #ifndef directive will never evaluate the macro's value and will always skip the TB formatting code. Use #if !SGL_EMSCRIPTEN to conditionally include TB support on native (non-Emscripten) builds.
Proposed fix
-#ifndef SGL_EMSCRIPTEN
+#if !SGL_EMSCRIPTEN
else if (size < 1099511627776ull)
return fmt::format("{:.2f} GB", size / 1073741824.0);
else
return fmt::format("{:.2f} TB", size / 1099511627776.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/core/string.cpp` around lines 122 - 130, The conditional uses
"#ifndef SGL_EMSCRIPTEN" which is wrong because SGL_EMSCRIPTEN is always
defined; change the preprocessor check to use "#if !SGL_EMSCRIPTEN" so the TB
branch (the fmt::format("{:.2f} TB", size / 1099511627776.0) return) is included
on native builds; update the directive surrounding the GB/TB returns in
src/sgl/core/string.cpp (the block that currently returns GB in the Emscripten
case and TB for large sizes) to use `#if` !SGL_EMSCRIPTEN / `#else` / `#endif` instead
of `#ifndef`.
| #elif SGL_EMSCRIPTEN | ||
| handle.canvasSelector = "#canvas"; | ||
| #endif |
There was a problem hiding this comment.
Hardcoded "#canvas" selector couples SGL to a fixed DOM id.
This ties every Window instance in a WASM build to <element id="canvas"> in the host page (as set up in examples/wasm/render_pipeline.html). It precludes multiple windows and makes the example HTML's id a load-bearing contract with the library.
Consider threading the selector through WindowDesc (e.g. a new optional canvas_selector field or window_handle override) so applications can choose their own canvas element, defaulting to "#canvas" for backwards compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/core/window.cpp` around lines 420 - 422, The code hardcodes "#canvas"
into handle.canvasSelector for SGL_EMSCRIPTEN; add an optional canvas_selector
(or canvasSelector) field to WindowDesc and make the Window
creation/initialization (where handle.canvasSelector is assigned) use
WindowDesc.canvas_selector if present, otherwise fall back to "#canvas" for
backward compatibility; update any Window/WindowDesc constructors or factory
functions that build WindowHandle to accept and propagate this new field so
multiple WASM canvases can be targeted.
| #elif SGL_EMSCRIPTEN | ||
| rhi::WindowHandle rhi_window_handle = rhi::WindowHandle::fromWGPUCanvas(window_handle.canvasSelector); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the surface.cpp file around lines 26-27
cat -n src/sgl/device/surface.cpp | sed -n '20,35p'Repository: shader-slang/slangpy
Length of output: 1052
🏁 Script executed:
# Find where rhi::WindowHandle and fromWGPUCanvas are defined
find . -type f \( -name "*.cpp" -o -name "*.h" -o -name "*.hpp" \) | xargs grep -l "fromWGPUCanvas" | head -20Repository: shader-slang/slangpy
Length of output: 92
🏁 Script executed:
# Search for the WindowHandle class definition
find . -type f \( -name "*.h" -o -name "*.hpp" \) | xargs grep -l "class WindowHandle\|struct WindowHandle" | head -10Repository: shader-slang/slangpy
Length of output: 89
🏁 Script executed:
# Look for rhi namespace and WindowHandle
rg "fromWGPUCanvas" -A 10 -B 2Repository: shader-slang/slangpy
Length of output: 1160
🏁 Script executed:
# Look for WindowHandle in rhi (likely in slang or external library)
rg "class.*WindowHandle|struct.*WindowHandle" -B 2 -A 15 --type cpp --type hRepository: shader-slang/slangpy
Length of output: 2024
🏁 Script executed:
# Check platform.h for WindowHandle structure
cat -n src/sgl/core/platform.h | head -100Repository: shader-slang/slangpy
Length of output: 3527
🏁 Script executed:
# Search for rhi:: namespace definitions
find . -type f \( -name "*.h" -o -name "*.hpp" \) -path "*/rhi/*" | head -20Repository: shader-slang/slangpy
Length of output: 46
🏁 Script executed:
# Look for fromWGPUCanvas implementation more broadly
rg "fromWGPUCanvas" --type cpp --type h -B 5 -A 15Repository: shader-slang/slangpy
Length of output: 1640
🏁 Script executed:
# Check if Slang is a git submodule
cat .gitmodules 2>/dev/null || echo "No .gitmodules found"Repository: shader-slang/slangpy
Length of output: 977
🏁 Script executed:
# Look for Slang RHI headers in the repo
find . -type d -name "rhi" | head -20Repository: shader-slang/slangpy
Length of output: 46
🏁 Script executed:
# Check if there's a Slang source directory
ls -la | grep -i slang || echo "No obvious slang directory"Repository: shader-slang/slangpy
Length of output: 123
🏁 Script executed:
# Search for where rhi:: is used and any related includes
rg "rhi::" -B 5 --type cpp | head -100Repository: shader-slang/slangpy
Length of output: 5627
🏁 Script executed:
# Check surface.cpp includes to understand rhi dependency
head -30 src/sgl/device/surface.cppRepository: shader-slang/slangpy
Length of output: 1064
🏁 Script executed:
# Check if slang-rhi submodule is present and look for WindowHandle
ls -la external/slang-rhi/Repository: shader-slang/slangpy
Length of output: 174
🏁 Script executed:
# Search for fromWGPUCanvas in slang-rhi
find external/slang-rhi -type f \( -name "*.h" -o -name "*.hpp" \) -o -name "*.cpp" | head -20Repository: shader-slang/slangpy
Length of output: 46
🏁 Script executed:
# Look for WindowHandle definition in slang-rhi
rg "fromWGPUCanvas" external/slang-rhi/ -B 5 -A 15Repository: shader-slang/slangpy
Length of output: 46
🏁 Script executed:
# Also look for WindowHandle class in slang-rhi
rg "class WindowHandle|struct WindowHandle" external/slang-rhi/ -B 3 -A 20Repository: shader-slang/slangpy
Length of output: 46
🏁 Script executed:
# Check if there are any comments or documentation about fromWGPUCanvas in the codebase
rg "WGPUCanvas|canvas.*selector" -i --type cpp --type h | head -30Repository: shader-slang/slangpy
Length of output: 326
🏁 Script executed:
# Look for any error handling patterns in surface.cpp
cat -n src/sgl/device/surface.cppRepository: shader-slang/slangpy
Length of output: 4023
🏁 Script executed:
# Search for how null validation is done elsewhere in similar code
rg "SGL_THROW\|SGL_ASSERT" src/sgl/device/ -B 2 -A 2 | head -60Repository: shader-slang/slangpy
Length of output: 46
Add null validation for Emscripten canvas selector.
The public Surface(WindowHandle, ref<Device>) constructor can receive a default-initialized WindowHandle{} where canvasSelector == nullptr, which is then passed unchecked to rhi::WindowHandle::fromWGPUCanvas(). While the Window* constructor path supplies "#canvas" safely, direct instantiation bypasses this protection.
Suggested guard
`#elif` SGL_EMSCRIPTEN
+ if (!window_handle.canvasSelector)
+ SGL_THROW("Emscripten surface creation requires a non-null canvas selector.");
rhi::WindowHandle rhi_window_handle = rhi::WindowHandle::fromWGPUCanvas(window_handle.canvasSelector);
`#endif`📝 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.
| #elif SGL_EMSCRIPTEN | |
| rhi::WindowHandle rhi_window_handle = rhi::WindowHandle::fromWGPUCanvas(window_handle.canvasSelector); | |
| `#elif` SGL_EMSCRIPTEN | |
| if (!window_handle.canvasSelector) | |
| SGL_THROW("Emscripten surface creation requires a non-null canvas selector."); | |
| rhi::WindowHandle rhi_window_handle = rhi::WindowHandle::fromWGPUCanvas(window_handle.canvasSelector); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/device/surface.cpp` around lines 26 - 27, The Surface(WindowHandle,
ref<Device>) constructor passes window_handle.canvasSelector directly into
rhi::WindowHandle::fromWGPUCanvas without checking for null; add a guard in the
Surface constructor to detect a null or empty canvasSelector on the incoming
WindowHandle and substitute a safe default (e.g. "#canvas") before calling
rhi::WindowHandle::fromWGPUCanvas. Locate the branch for SGL_EMSCRIPTEN in
surface.cpp where
rhi::WindowHandle::fromWGPUCanvas(window_handle.canvasSelector) is used and wrap
it so that if window_handle.canvasSelector is null/empty you call fromWGPUCanvas
with the default selector instead.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/sgl/device/device.cpp (3)
1346-1358:⚠️ Potential issue | 🟡 MinorFail fast when CUDA context handles are requested without CUDA support.
Returning default/invalid handles when
SGL_HAS_CUDAis false can propagate hard-to-debug failures to callers. Prefer throwing an explicit error instead.Suggested fix
std::array<NativeHandle, 3> get_cuda_current_context_native_handles() { std::array<NativeHandle, 3> handles; `#if` SGL_HAS_CUDA CUcontext cu_context; SGL_CHECK(rhiCudaDriverApiInit(), "Failed to initialize CUDA driver API."); SGL_CU_CHECK(cuCtxGetCurrent(&cu_context)); SGL_CHECK(cu_context, "No current CUDA context found."); @@ handles[0] = NativeHandle(cu_device); handles[1] = NativeHandle(cu_context); +#else + SGL_THROW("CUDA is not available in this build."); `#endif` return handles; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/device/device.cpp` around lines 1346 - 1358, The code returns default/invalid NativeHandle values when SGL_HAS_CUDA is false; change this to fail fast by adding an `#else` branch that throws an explicit error (e.g., throw std::runtime_error("CUDA support not compiled in; cannot return CUDA handles") or use the project’s SGL_CHECK/SGL_ERROR macro) instead of returning handles, referencing the handled symbols NativeHandle and the handles array (and existing SGL_CHECK/SGL_CU_CHECK usage) so callers get a clear immediate failure when CUDA context/device handles are requested without CUDA support.
869-905:⚠️ Potential issue | 🟠 MajorDo not silently ignore
cuda_streamin non-CUDA builds.With
SGL_HAS_CUDAdisabled, a validcuda_streamcan currently be accepted and then dropped by forcing.cudaStream = nullptr. This should be rejected to avoid silent misconfiguration.Suggested fix
SGL_CHECK( !cuda_stream.is_valid() || cuda_stream.type() == NativeHandleType::CUstream, "Native handle supplied for CUDA stream is not of type CUstream." ); + +#if !SGL_HAS_CUDA + SGL_CHECK(!cuda_stream.is_valid(), "CUDA stream is not supported in builds without CUDA support."); +#endifAlso applies to: 945-949
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/device/device.cpp` around lines 869 - 905, When SGL_HAS_CUDA is disabled the code currently drops a valid cuda_stream (silently accepting and setting cuda_stream_ptr=null), so add an explicit rejection: where the code branches based on SGL_HAS_CUDA and m_supports_cuda_interop, check cuda_stream.is_valid() in the non-CUDA build/path and fail fast with SGL_CHECK (or similar) instead of silently ignoring it. Concretely, update the logic around cuda_stream/cuda_stream_ptr (symbols: cuda_stream, cuda_stream_ptr, m_desc.type, DeviceType::cuda, m_supports_cuda_interop, needs_cuda_sync, sync_to_cuda, copy_from_cuda) so that when SGL_HAS_CUDA is not defined and CUDA interop is not actually available any valid cuda_stream triggers SGL_CHECK("CUDA stream is not supported on this device.") (also apply the same guard at the other occurrence referenced in the comment).
133-205:⚠️ Potential issue | 🟠 MajorHandle
enable_cuda_interopexplicitly when CUDA is not compiled in.When
SGL_HAS_CUDAis false, this whole init path is removed, butm_desc.enable_cuda_interopcan remain true and still affect behavior later (for example shared fence creation at Line 364). Please fail fast or disable the flag in non-CUDA builds.Suggested fix
+ `#if` !SGL_HAS_CUDA + if (m_desc.enable_cuda_interop) { + m_desc.enable_cuda_interop = false; + log_warn("CUDA interop requested, but this build has no CUDA support. Disabling enable_cuda_interop."); + } + `#endif` + `#if` SGL_HAS_CUDA // If CUDA interop is enabled on non-cuda backend, check if existing CUDA context or device // is provided. If so, we will attempt to identify the same device for use with SlangPy. if (m_desc.enable_cuda_interop) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/device/device.cpp` around lines 133 - 205, When SGL_HAS_CUDA is not defined, explicitly handle m_desc.enable_cuda_interop to avoid later surprises: at the start of the CUDA-init branch (or earlier in Device initialization) detect if m_desc.enable_cuda_interop is true and either call close() followed by SGL_THROW with a clear message that CUDA interop was requested but the build lacks CUDA support, or set m_desc.enable_cuda_interop = false and emit a log_warn explaining CUDA interop is disabled in this build; update the code around the existing SGL_HAS_CUDA / CUDA initialization block that references m_desc.enable_cuda_interop to implement this fail-fast or safe-noop behavior.
♻️ Duplicate comments (1)
src/sgl/core/window.cpp (1)
420-422:⚠️ Potential issue | 🟡 MinorAvoid hardcoding the canvas selector.
This still couples every WASM window to a fixed
#canvasDOM id. If the host page uses a different element or needs more than one canvas, surface creation will fail. This is the same issue raised in the previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/core/window.cpp` around lines 420 - 422, The code currently hardcodes handle.canvasSelector = "#canvas" under SGL_EMSCRIPTEN; change this to use a configurable value instead: expose a parameter or config field (e.g., a constructor/initializer argument or WindowOptions) that supplies the canvas selector and fall back to "#canvas" only as the default, and update the code that initializes the window handle (the place assigning handle.canvasSelector and any callers of the window creation/initialization functions) to read the provided selector; keep the SGL_EMSCRIPTEN conditional but use the configurable value rather than a fixed string so hosts can pass different selectors or multiple canvases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/sgl/device/device.cpp`:
- Around line 1346-1358: The code returns default/invalid NativeHandle values
when SGL_HAS_CUDA is false; change this to fail fast by adding an `#else` branch
that throws an explicit error (e.g., throw std::runtime_error("CUDA support not
compiled in; cannot return CUDA handles") or use the project’s
SGL_CHECK/SGL_ERROR macro) instead of returning handles, referencing the handled
symbols NativeHandle and the handles array (and existing SGL_CHECK/SGL_CU_CHECK
usage) so callers get a clear immediate failure when CUDA context/device handles
are requested without CUDA support.
- Around line 869-905: When SGL_HAS_CUDA is disabled the code currently drops a
valid cuda_stream (silently accepting and setting cuda_stream_ptr=null), so add
an explicit rejection: where the code branches based on SGL_HAS_CUDA and
m_supports_cuda_interop, check cuda_stream.is_valid() in the non-CUDA build/path
and fail fast with SGL_CHECK (or similar) instead of silently ignoring it.
Concretely, update the logic around cuda_stream/cuda_stream_ptr (symbols:
cuda_stream, cuda_stream_ptr, m_desc.type, DeviceType::cuda,
m_supports_cuda_interop, needs_cuda_sync, sync_to_cuda, copy_from_cuda) so that
when SGL_HAS_CUDA is not defined and CUDA interop is not actually available any
valid cuda_stream triggers SGL_CHECK("CUDA stream is not supported on this
device.") (also apply the same guard at the other occurrence referenced in the
comment).
- Around line 133-205: When SGL_HAS_CUDA is not defined, explicitly handle
m_desc.enable_cuda_interop to avoid later surprises: at the start of the
CUDA-init branch (or earlier in Device initialization) detect if
m_desc.enable_cuda_interop is true and either call close() followed by SGL_THROW
with a clear message that CUDA interop was requested but the build lacks CUDA
support, or set m_desc.enable_cuda_interop = false and emit a log_warn
explaining CUDA interop is disabled in this build; update the code around the
existing SGL_HAS_CUDA / CUDA initialization block that references
m_desc.enable_cuda_interop to implement this fail-fast or safe-noop behavior.
---
Duplicate comments:
In `@src/sgl/core/window.cpp`:
- Around line 420-422: The code currently hardcodes handle.canvasSelector =
"#canvas" under SGL_EMSCRIPTEN; change this to use a configurable value instead:
expose a parameter or config field (e.g., a constructor/initializer argument or
WindowOptions) that supplies the canvas selector and fall back to "#canvas" only
as the default, and update the code that initializes the window handle (the
place assigning handle.canvasSelector and any callers of the window
creation/initialization functions) to read the provided selector; keep the
SGL_EMSCRIPTEN conditional but use the configurable value rather than a fixed
string so hosts can pass different selectors or multiple canvases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02cbe91b-7381-48a7-b9d2-5f5be8b10999
📒 Files selected for processing (3)
src/sgl/core/window.cppsrc/sgl/device/device.cppsrc/sgl/device/device.h
|
Worth reading through some of code rabbit's points as some seem valid - could you either address them or mark resolved. For thread.h, the last time I looked at WASM threads were still a proposal. If that's not changed I guess this needs to be pretty limited to raising errors where separate threads are attempted to be spawned. |
|
Sure. The comments are mostly okay, I need to find time to address them. |
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
* Test format acknowledgement * Apply automatic formatting * Include pre-commit version in failure comment --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ev/ccummings/native-slangpy
* Update slang-rhi * Fix formatting (unrelated)
5 participants
This is a draft PR where SGL compiles using Emscripten.
It relies on RHI PR to be merged: shader-slang/slang-rhi#609 and Slang's PR shader-slang/slang#9818 to produce WASM libraries to be automatically used for RHI/SGL compilation
Summary by CodeRabbit
New Features
Build System
Chores