- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 23.5k
 
          Make the behavior of rendering/rendering_device/driver consistent with --rendering-device
          #109207
        
          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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me.
aaa4262    to
    e819f49      
    Compare
  
    | 
           Has anyone had trouble with this, or are we just making this change because we can? To me to seems incredibly confusing to give the users the option to set a rendering driver that we know doesn't exist. It's different with command line arguments because we can't give immediate feedback. With project settings, we can immediately tell the user that the value they input is invalid if the property accepts a string, or not provide the option if we can provide a fixed list.  | 
    
          
 As I mentioned, it's not a conventional usage. In my own application, I provide a fixed list, but certain edge cases require users to manually modify the configuration file (for example, using  Even without this PR, the valid options still include  Currently, it indeed lacks a check. I can add one—should I?  | 
    
e819f49    to
    ac69f50      
    Compare
  
    
          
 Note that the engine currently doesn't provide this list for projects to use, but #109247 makes it possible.  | 
    
dc65c51    to
    500befd      
    Compare
  
    c806140    to
    098321b      
    Compare
  
    | 
           @clayjohn How to promote the merging of this PR?  | 
    
          
 You can make a proposal and see if other people have the same use-case as you.  | 
    
          
 However, I believe this PR also falls under bug fixes. Currently,   | 
    
36ac3c6    to
    c132bc8      
    Compare
  
    6525267    to
    1cf7bea      
    Compare
  
    1cf7bea    to
    e2cd09a      
    Compare
  
    3c68fdc    to
    7f21e03      
    Compare
  
    cf7163c    to
    975509b      
    Compare
  
    9d4d51b    to
    02e8053      
    Compare
  
    cf1e9e8    to
    abf1f9f      
    Compare
  
    bc96b2b    to
    879cc02      
    Compare
  
    …ith `--rendering_device`
879cc02    to
    70c91ff      
    Compare
  
    
Currently, the

--rendering-devicecommand supports direct switching betweend3d12,vulkan, andopengl3, among others. Although therendering/rendering_device/driversetting in the configuration file can also be set toopengl3, doing so may cause rendering bugs in child windows:Through examining the source code, I discovered that this occurs because when reading
--rendering-device, ifrendering_driveris set toopengl3, it automatically setsrendering_methodtogl_compatibility. However, reading from the configuration file does not trigger this behavior, so I added it manually.This is not the conventional usage of
rendering/rendering_device/driver, but I'd like to allow users to switch rendering APIs viaoverride.cfgin my project. Whether it'sd3d12,vulkan, oropengl3, different users have reported rendering issues with each. Previously, users could only switch APIs using--rendering-device, which wasn't very convenient. Enablingrendering/rendering_device/driverto be properly set toopengl3would be much more user-friendly (sparing users the need to simultaneously change bothrendering/renderer/rendering_methodandrendering/rendering_device/driver).