Initial splash screen implementation#1957
Conversation
WalkthroughAdds a splash-screen path: a private Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant ExecutionFlow
participant GUI
participant GL as Textures
App->>ExecutionFlow: Run()
ExecutionFlow->>GUI: StartFrame()/Init()
alt first frame & m_enableSplashScreen == true
GUI->>GUI: getSplashScreen(w,h) -> unique_ptr(data)
GUI->>GL: bind(offscreen tex A)
GUI->>GL: glTexSubImage2D(data)
GUI->>GL: set TEXTURE_MIN/MAG_FILTER = NEAREST
GUI->>GL: bind(offscreen tex B)
GUI->>GL: glTexSubImage2D(data)
GUI->>App: flip() (present)
else subsequent frames
GUI->>GL: render normal frame content to offscreen textures
GUI->>App: flip()
end
ExecutionFlow->>ExecutionFlow: m_enableSplashScreen = false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Areas to review closely:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/gui/gui.cc (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/gui/gui.h (1)
461-462: Consider annotating getSplashScreen for intent and misuse preventionOptional refinements:
- Mark as [[nodiscard]] to prevent accidental ignoring of the returned buffer.
- Mark noexcept if you’re confident stbir won’t throw in this context (it shouldn’t).
- Since it doesn’t depend on object state, consider making it static.
- std::unique_ptr<uint32_t[]> getSplashScreen(uint32_t destWidth, uint32_t destHeight); + [[nodiscard]] static std::unique_ptr<uint32_t[]> getSplashScreen(uint32_t destWidth, uint32_t destHeight) noexcept;tools/splash-generator/splash.py (3)
1-1: Use python3 in the shebang for portabilityMany environments map python to Python 2 or don’t provide it at all. Using python3 avoids ambiguity.
-#!/usr/bin/env python +#!/usr/bin/env python3
12-15: Use a context manager to ensure the image file is closed and add basic error handlingThis avoids leaking file descriptors on exceptions and provides a clearer error if Pillow can’t open the file.
-img = Image.open(filename).convert("RGBA") -width, height = img.size -pixels = list(img.getdata()) +try: + with Image.open(filename) as img: + img = img.convert("RGBA") + width, height = img.size + pixels = list(img.getdata()) +except Exception as e: + print(f"Error: failed to open or process image '{filename}': {e}", file=sys.stderr) + sys.exit(2)
25-25: Remove extraneous f-string prefix (Ruff F541)There are no placeholders; drop the f to satisfy linters and avoid confusion.
-print(f"static const uint32_t s_splashImageData[] = {{") +print("static const uint32_t s_splashImageData[] = {")src/gui/gui.cc (1)
956-969: Texture parameterization before data upload fixes the macOS driver issueSetting GL_TEXTURE_MAG_FILTER/MIN_FILTER to GL_NEAREST before calling glTexSubImage2D addresses the Apple driver error where writes occur before filters are set. Uploading the splash to both offscreen textures is sound.
Two small optional improvements:
- Avoid recomputing the resized splash if m_setupScreenSize toggles repeatedly (e.g., during window resizing): cache the last (w,h) and the buffer to reuse within a frame or until size changes.
- Consider documenting that glTexImage2D is used solely to allocate storage (pixels=0), so it’s not considered a “write” in the context of the driver issue.
- std::unique_ptr<uint32_t[]> splashImageData = getSplashScreen(m_renderSize.x, m_renderSize.y); + // Optional: cache the resized splash per (w,h) to avoid recompute storms during resize. + std::unique_ptr<uint32_t[]> splashImageData = getSplashScreen(m_renderSize.x, m_renderSize.y);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/gui/gui.cc(3 hunks)src/gui/gui.h(2 hunks)tools/splash-generator/splash.py(1 hunks)vsprojects/gui/gui.vcxproj(1 hunks)vsprojects/gui/gui.vcxproj.filters(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/gui/gui.h (1)
src/gui/splash.cc (2)
getSplashScreen(38411-38424)getSplashScreen(38411-38411)
src/gui/gui.cc (1)
src/gui/splash.cc (2)
getSplashScreen(38411-38424)getSplashScreen(38411-38411)
🪛 Ruff (0.12.2)
tools/splash-generator/splash.py
25-25: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: build
- GitHub Check: cross-arm64
- GitHub Check: asan
🔇 Additional comments (6)
vsprojects/gui/gui.vcxproj.filters (1)
106-108: Include of splash.cc in filters looks correctThe new translation unit is properly categorized under "Source Files". This aligns with the project file change and will make the file show up correctly in the VS Solution Explorer.
src/gui/gui.h (2)
28-28: Include is appropriate for unique_ptr usageThis is needed for the new helper returning std::unique_ptr. No issues here.
458-458: m_enableSplashScreen defaulting to true fits the startup-flowThis keeps the splash visible until emulation starts (disabled on Run). Looks good.
vsprojects/gui/gui.vcxproj (1)
131-131: splash.cc added to the GUI buildThis ensures the splash functionality is compiled in all configurations. Good.
tools/splash-generator/splash.py (1)
16-21: Endianness and channel order note (FYI)Packing as 0xAABBGGRR for each pixel is correct for little-endian RGBA8888 consumption (bytes in memory: R G B A). This matches the downstream use with STBIR_RGBA and GL_RGBA/GL_UNSIGNED_BYTE.
src/gui/gui.cc (1)
556-559: Disable splash on Run event is correct and low-riskThis ensures the splash is only shown prior to execution. Clean, simple, and consistent with the PR description.
|
Looks like this breaks nix. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pcsx-redux.nix(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-11T16:27:49.966Z
Learnt from: NotExactlySiev
Repo: grumpycoders/pcsx-redux PR: 1853
File: pcsx-redux.nix:162-164
Timestamp: 2025-02-11T16:27:49.966Z
Learning: In pcsx-redux's Nix configuration, zlib will be needed as a runtime dependency for Lua functionality in the future, hence the commented placeholder in runtimeDependencies.
Applied to files:
pcsx-redux.nix
📚 Learning: 2025-02-11T16:27:22.888Z
Learnt from: NotExactlySiev
Repo: grumpycoders/pcsx-redux PR: 1853
File: 001-patch.diff:92-97
Timestamp: 2025-02-11T16:27:22.888Z
Learning: Lua initialization is temporarily disabled in the Nix build of pcsx-redux (commented out in src/main/main.cc) and will be fixed in a future update. This is an intentional state during the Nix support implementation.
Applied to files:
pcsx-redux.nix
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: aur-build
- GitHub Check: build
| ({ | ||
| owner = "nothings"; | ||
| repo = "stb"; | ||
| rev = "ae721c50eaf761660b4f90cc590453cdb0c2acd0"; | ||
| hash = "sha256-BIhbhXV7q5vodJ3N14vN9mEVwqrP6z9zqEEQrfLPzvI="; | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Investigate the Nix build breakage.
The stb submodule entry structure is correct and follows the same pattern as other submodules. However, the PR comment explicitly states "Looks like this breaks nix."
The potential issues could be:
- Incorrect revision or hash causing a fetch failure
- Build system not finding stb headers at
source/third_party/stb - Missing build configuration to integrate the stb submodule
Please run the following script to verify the stb submodule configuration:
Additionally, please check the actual Nix build error logs to identify the specific failure point.
🏁 Script executed:
#!/bin/bash
# Verify the stb revision exists and check Nix build configuration
# Check if the stb revision exists in the repository
echo "Checking stb revision..."
curl -s "https://api.github.com/repos/nothings/stb/commits/ae721c50eaf761660b4f90cc590453cdb0c2acd0" | jq -r '.sha // "NOT FOUND"'
# Search for stb usage in build files
echo -e "\nSearching for stb references in build configuration..."
rg -n "stb" --type=make -C2
# Search for stb includes in source files
echo -e "\nSearching for stb header includes..."
rg -n "#include.*stb" -C2
# Check if there are any Nix-specific build errors or configurations
echo -e "\nSearching for Nix build references to stb..."
rg -n "stb" -g "*.nix" -C3Length of output: 1602
🏁 Script executed:
#!/bin/bash
# Investigate build configuration and stb integration
# Look for CMakeLists.txt or other build files
echo "=== Looking for build configuration files ==="
fd -e cmake -e "CMakeLists.txt" | head -20
# Search for include_directories or add_include_directories in CMake
echo -e "\n=== Searching for include_directories configuration ==="
rg -n "include_directories|target_include_directories" -C2 | head -40
# Look for third_party in CMakeLists files
echo -e "\n=== Checking third_party references in build files ==="
rg -n "third_party" -g "CMakeLists.txt" -C2
# Check if there's a stb directory already
echo -e "\n=== Checking for stb directory ==="
fd -t d "stb$" 2>/dev/null
# Look at how third_party/clip is configured (since it uses stb)
echo -e "\n=== Checking third_party structure and dependencies ==="
ls -la third_party/ 2>/dev/null | head -20
# Check if there's any stb configuration or build instructions
echo -e "\n=== Looking for stb-related build configuration ==="
rg -n "stb" --type=cmake -C2Length of output: 4634
🏁 Script executed:
#!/bin/bash
# Deep dive into stb directory and build configuration
# Check what's in the stb directory
echo "=== Contents of third_party/stb/ ==="
ls -la third_party/stb/ 2>/dev/null || echo "Directory not found or empty"
# Check size and file count
echo -e "\n=== stb directory size and file count ==="
find third_party/stb/ -type f 2>/dev/null | wc -l
du -sh third_party/stb/ 2>/dev/null
# Look for any build files at repo root
echo -e "\n=== Looking for root-level build configuration ==="
ls -la | grep -E "CMakeLists|meson|setup|configure|Makefile|build" || echo "No standard build files found"
# Check the pcsx-redux.nix more carefully around fetchSubmodule and postUnpack
echo -e "\n=== Examining pcsx-redux.nix structure (around submodule handling) ==="
sed -n '105,130p' pcsx-redux.nix
# Look for how stb was previously handled (if any history)
echo -e "\n=== Checking if stb is referenced in any other Nix config ==="
rg -n "stb" -g "*.nix" -A2 -B2
# Check if stb headers actually exist in the submodule
echo -e "\n=== Checking for stb header files ==="
find third_party/stb -name "stb_*.h" 2>/dev/null | head -10Length of output: 1563
🏁 Script executed:
#!/bin/bash
# Check Makefile and build configuration for stb handling
# Search Makefile for stb references
echo "=== Searching Makefile for stb references ==="
rg -n "stb" Makefile -C2
# Check how include paths are configured
echo -e "\n=== Checking include path configuration in Makefile ==="
rg -n "INCLUDE|CFLAGS|CPPFLAGS" Makefile -C1 | head -40
# Look for third_party references in Makefile
echo -e "\n=== Checking third_party references in Makefile ==="
rg -n "third_party" Makefile -C1
# Check if there's a specific build config for headers
echo -e "\n=== Looking for include directory setup ==="
rg -n "source/third_party|third_party" Makefile | head -20
# Verify the hash by checking if repo fetch would work (check GitHub)
echo -e "\n=== Testing GitHub API for stb repo ==="
curl -s "https://api.github.com/repos/nothings/stb/contents/" | jq -r '.[0:3] | .[] | .name' 2>/dev/null || echo "API call failed or no results"
# Look for any errors or comments in the PR/Nix about this change
echo -e "\n=== Full submodules section in pcsx-redux.nix ==="
sed -n '80,105p' pcsx-redux.nixLength of output: 10991
The stb submodule is not being fetched, causing build failure.
The third_party/stb/ directory is empty despite the Nix submodule entry. The build will fail because the code includes stb headers (third_party/clip/image.cpp, third_party/ImFileDialog/ImFileDialog.cpp, and src/gui/splash.cc all require stb headers), but the files won't exist in the expected location.
The submodule entry structure is correct, but the fetchFromGitHub call is not actually copying files into source/third_party/stb. Verify the hash is correct for the specified revision, or check the actual Nix build logs to see if fetchFromGitHub is failing silently.
There was a problem hiding this comment.
@NotExactlySiev, understood. I'll step back on this issue.
🧠 Learnings used
Learnt from: NotExactlySiev
Repo: grumpycoders/pcsx-redux PR: 1853
File: pcsx-redux.nix:162-164
Timestamp: 2025-02-11T16:27:49.966Z
Learning: In pcsx-redux's Nix configuration, zlib will be needed as a runtime dependency for Lua functionality in the future, hence the commented placeholder in runtimeDependencies.
Learnt from: NotExactlySiev
Repo: grumpycoders/pcsx-redux PR: 1853
File: 001-patch.diff:92-97
Timestamp: 2025-02-11T16:27:22.888Z
Learning: Lua initialization is temporarily disabled in the Nix build of pcsx-redux (commented out in src/main/main.cc) and will be fixed in a future update. This is an intentional state during the Nix support implementation.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Should be good to go I think |
3 participants
The current splash screen is a sample and not necessarily final. This code will also likely be replaced when nanovg/nanosvg are replaced. Also worth noting that this relies on stb-image-resize2 for resizing the splash image to the render resolution. This can be avoided, but might require adding a fair bit of code.
Screen.Recording.2025-08-15.at.2.54.46.PM.mov
Image used for the splash screen in the video (Made in Canva, so I sadly can't share the project files easily)
