Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sets 'requiresRestart: false' instead of 'true' for some emuThree core options #2379

Merged
merged 1 commit into from
Jan 12, 2025

Conversation

yippeeeyay
Copy link
Contributor

@yippeeeyay yippeeeyay commented Jan 12, 2025

User description

What does this PR do

I noticed that most options that have 'requiresRestart: true' do not actually apply on restart. These changes should (in theory) fix resolution upscaling not updating, cpu clock not updating, as well as some other core options. On Citra, most settings do not require a restart anyway, except for JIT.

I also added a description for Shader Accurate Mul.

Where should the reviewer start

How should this be manually tested

This needs to be tested as I cannot build emuThree for myself. Resolution upscaling and cpu clock speed are the most important ones to check if they update while in game. Other options that I set requiresRestart to false are logging, new 3ds, async shader comp, and shader accurate mul.

Any background context you want to provide

What are the relevant tickets

Screenshots (important for UI changes)

Questions


PR Type

Enhancement


Description

  • Changed requiresRestart to false for several core options.

  • Updated descriptions for CPU Clock Speed and Shader Accurate Mul.

  • Improved user experience by reducing unnecessary restart requirements.

  • Enhanced clarity of core option descriptions.


Changes walkthrough 📝

Relevant files
Enhancement
PVEmuThreeCoreOptions.swift
Updated core options to avoid restarts and improve descriptions

Cores/emuThree/PVEmuThreeCore/Core/PVEmuThreeCoreOptions.swift

  • Changed requiresRestart to false for multiple core options.
  • Updated description for CPU Clock Speed option.
  • Added description for Shader Accurate Mul option.
  • Improved user experience by reducing restart requirements.
  • +10/-10 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    …ds description for Shader Accurate Mul
    
    I noticed that most options that have 'requiresRestart: true' do not actually apply on restart. These changes should (in theory) fix upscaling not updating, cpu clock not updating, as well as some other core options. On Citra, most settings do not require a restart anyway, except for JIT.
    
    This needs to be tested as I cannot build emuThree for myself.
    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Validation Needed

    The changes to requiresRestart flags need thorough testing to verify that settings like resolution upscaling and CPU clock speed properly update during gameplay without requiring restart

    description: nil,
    requiresRestart: false),
    Performance Impact

    Setting enableLogging and enableAsyncShaderOption to not require restart could potentially cause performance issues if toggled during gameplay

            requiresRestart: false),
              defaultValue: false)
    }
    
    static var enableNew3DSOption: CoreOption {
        .bool(.init(
            title: "Enable New 3DS",
            description: nil,
            requiresRestart: false),
              defaultValue: true)
    }
    
    static var gsOption: CoreOption {
        .enumeration(.init(title: "Graphics Handler",
                           description: "(Requires Restart)",
                           requiresRestart: true),
                     values: [
                        .init(title: "Vulkan", description: "Vulkan", value: 0),
                     ],
                     defaultValue: 0)
    }
    
    static var enableAsyncShaderOption: CoreOption {
        .bool(.init(
            title: "Enable Async Shader Compilation",
            description: nil,
            requiresRestart: false),

    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Maintain restart requirement for resolution changes to prevent potential graphics-related crashes

    Keep requiresRestart as true for Resolution Upscaling since changing resolution at
    runtime could lead to rendering issues or crashes. Resolution changes typically need
    a full restart to properly reallocate graphics resources.

    Cores/emuThree/PVEmuThreeCore/Core/PVEmuThreeCoreOptions.swift [8-10]

     .enumeration(.init(title: "Resolution Upscaling",
    -                       description: nil,
    -                       requiresRestart: false),
    +                       description: "Changes require restart to take effect",
    +                       requiresRestart: true),
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Resolution changes typically require graphics resource reallocation, and changing this at runtime could cause serious rendering issues or crashes. The suggestion correctly identifies a potential stability risk.

    9
    Maintain system stability by requiring restart when switching hardware emulation modes

    Keep requiresRestart as true for Enable New 3DS option since it fundamentally
    changes the emulated hardware configuration, which cannot be safely changed during
    runtime.

    Cores/emuThree/PVEmuThreeCore/Core/PVEmuThreeCoreOptions.swift [73-76]

     .bool(.init(
             title: "Enable New 3DS",
    -        description: nil,
    -        requiresRestart: false),
    +        description: "Changes require restart to take effect",
    +        requiresRestart: true),
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Switching between different hardware emulation modes (New 3DS vs Old 3DS) is a fundamental system change that could cause instability if modified during runtime. This suggestion correctly identifies a critical safety issue.

    9
    Ensure system stability by requiring restart for CPU clock speed modifications

    Keep requiresRestart as true for CPU Clock Speed adjustments since modifying the
    emulator's CPU frequency during runtime could lead to system instability and game
    crashes.

    Cores/emuThree/PVEmuThreeCore/Core/PVEmuThreeCoreOptions.swift [35-37]

     .enumeration(.init(title: "CPU Clock Speed",
    -                       description: "Underclocking can increase performance but may cause the game to freeze. Overclocking may reduce in game lag but also might cause freezes",
    -                       requiresRestart: false),
    +                       description: "Underclocking can increase performance but may cause the game to freeze. Overclocking may reduce in game lag but also might cause freezes. Changes require restart.",
    +                       requiresRestart: true),
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Modifying CPU clock speed during runtime could lead to system instability and game crashes. The suggestion correctly identifies this as a critical system-level setting that should require a restart.

    8

    @JoeMatt JoeMatt merged commit c54c2d4 into Provenance-Emu:develop Jan 12, 2025
    2 of 7 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants