- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 23.5k
 
Add ability to get list of Project Settings changed, similar to Editor Settings functionality #110748
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?
Add ability to get list of Project Settings changed, similar to Editor Settings functionality #110748
Conversation
| 
           I'm not too keen with this solution of having two very similar signal names for the same concept. In my opinion it makes more sense to introduce the  Thing is, I believe that would cause a breaking change for existing code, even though we are only adding arguments, since there would be a mismatch of argument count between the signal and existing connected functions. So I'm not sure how problematic this would be.  | 
    
| 
           I agree, I originally changed the existing signal, but I was trying to avoid a breaking change. I am happy to make the breaking change if that's the approach deemed correct. I could fix the broken code in the engine, but any userland code would still be affected. I'm not sure how widely used this signal is outside of addons, but anyone using this signal would benefit from the usability improvements, being able to filter on specific settings, and reducing processing, at the cost of a minor change to the signature.  | 
    
dcfeebf    to
    053f96d      
    Compare
  
    | 
           Needs a proposal:  | 
    
| 
           This will only emit for exactly one setting, which would be confusing IMO, it should at least be mentioned in the documentation that changing a second setting in the same frame will not trigger this specific signal  | 
    
          
 That was not the intended behavior, I must not have tested that as well as I thought. My aim is that each setting change would trigger this signal. I won't update this yet I'll wait on the proposal approval before spending more time on it.  | 
    
| 
           Based on how I read the code it queues one change notification per frame, for the first changed case, after that it skips any changes of any setting, as it sets   | 
    
053f96d    to
    e7dd3f8      
    Compare
  
            
          
                core/config/project_settings.cpp
              
                Outdated
          
        
      | signal.name = p_name; | ||
| signal.old_value = p_old_value; | ||
| signal.new_value = p_new_value; | ||
| pending_signals.push_back(signal); | 
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.
Looks better, note though that this will fire for duplicates, but that would be desirable I'd say, though if you do change the setting twice in a frame it will call with the two changed values in order
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.
I agree, multiple changes to the same settings for different values is correct to fire for each change.
Multiple changes to the same setting of the same value do not.
I can add an explicit test for that to make it clear.
b2d82e6    to
    b5582da      
    Compare
  
    | 
           I have reworked this to mimic the way Editor Settings work.  | 
    
b5582da    to
    482864c      
    Compare
  
            
          
                doc/classes/ProjectSettings.xml
              
                Outdated
          
        
      | Clears the whole configuration (not recommended, may break things). | ||
| </description> | ||
| </method> | ||
| <method name="clear_changed_settings"> | 
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.
What is this for?
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.
Leftover from a test method, cleanup and removed.
        
          
                doc/classes/ProjectSettings.xml
              
                Outdated
          
        
      | Returns the localized path (starting with [code]res://[/code]) corresponding to the absolute, native OS [param path]. See also [method globalize_path]. | ||
| </description> | ||
| </method> | ||
| <method name="mark_setting_changed"> | 
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.
I know EditorSettings also have this method, but I wonder why 🤔 Changing a setting will automatically mark it as modified.
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.
Honestly I dont know either.
I copied it to to match EditorSettings but I dont know how a setting could get in another way.
I have removed it.
        
          
                core/config/project_settings.cpp
              
                Outdated
          
        
      | // Capture old value before making changes | ||
| Variant old_value; | ||
| if (props.has(p_name)) { | ||
| old_value = props[p_name].variant; | 
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.
If you are checking anyway whether the value changed, maybe the method could return early if it didn't change?
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.
Good call I have added an early return check.
482864c    to
    07b8725      
    Compare
  
            
          
                core/config/project_settings.h
              
                Outdated
          
        
      | uint32_t _version = 1; | ||
| 
               | 
          ||
| // Track changed settings for get_changed_settings functionality | ||
| HashSet<String> changed_settings; | 
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.
The method to queue changes uses StringName and in _set it checks for has, so I'd say that using StringName here would be best and then just casting it to String when you need a String. It's not expensive but it will improve the checking operations and the insertion as StringName already has its hash and the comparison is faster
You'd just need to change the const String &setting : changed_settings to const String setting : changed_settings
StringName has a tiny overhead in storage but I'd say that's not critical compared with the better performance when inserting and querying
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.
OK I can make that change.
Do you think I should I change EditorSettings while I am at it to keep them in sync?
Or do that as a seperate performance improvement PR?
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.
I'd say to keep that as is, or leave it in both cases
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.
Added the change for ProjectSettings
07b8725    to
    e451640      
    Compare
  
    
Introduces new functions
get_changed_settingsandcheck_changed_settings_in_group. This brings Project Settings to parity with the Editor Settings functionality.This allows much more fine-grained responses to setting changes.
This approach is non-breaking.
As part of this, the
settings_changedis now only emitted when a settings value is set to a different value.Fixes: #110747
Closes godotengine/godot-proposals#13244