- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 23.5k
 
Add indicator to linked resources #109458
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
      
        
              This comment was marked as outdated.
        
        
      
    
  This comment was marked as outdated.
| 
           Please make sure your code is formatted properly locally before pushing again if you want checks to run, see here  | 
    
ea7d0bb    to
    f1ce477      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
          
 I do much prefer the counter - perhaps there's a middle ground that still offers one without the effort of tracking scene-external link counts. Maybe it could display the scene-local link count if it's internal, and then if it's external, make it instead read   | 
    
          
 
 Makes sense to take into account concerns raised in #102196: 
 
  | 
    
| 
           The icon for sub resources is more of a suggestion, one I'm particularly not that fond of. Maybe changing the theme of The Sub resource to indicate that? Or maybe adding a notification when trying to make that resource unique?  | 
    
| 
           I think the main (and most difficult) thing that should be done first it to detect amount of references to a resource. 
 (Please note that this is my personal opinion, and it may not coincide with the opinion of the Godot team members.)  | 
    
763256c    to
    d9c3e5b      
    Compare
  
    | 
           Edge Cases aside, I'm done with this PR and it's ready for review! (I'm tired help)  | 
    
a61f0e0    to
    44af052      
    Compare
  
    | 
           Hey everyone, small update: Reinforced Support for Subresources, so they're taken into account when making external ones unique, and also when they're deleted with the parent. Arrays/Dictionaries are also using the counter now. Note (mostly to self): Subresources in this PR are treated as inseparable from their parents, as it's impossible for a Resource and its nested child not have the same Nodes referencing them. So I took advantage of this when needed to keep better track of Subresources' counter. Also, dont use Vscode for git, it suckssss  | 
    
55cd5f1    to
    a0e9cb4      
    Compare
  
    | 
           I did a code review. I didn't delve very deep into the implementation, at glance it looks ok. It's limited only to editor code, which is good. The only concern is scanning all properties whenever object/array/dictionary is changed. I did test some cases that I think could be heavily affected and didn't notice any big slowdowns, so it's not as bad as it looks. Also the code duplication. OBJECT/ARRAY/DICTIONARY is a famous trio in inspector-related code. I've seen the same methods many times already, but since they always do something a bit different, it's difficult to really unify it :/  | 
    
| 
           Thanks for you guys Input! 
 This is most likely due to how External Resources are checked for recursive uniqueness, I'll take a look on that. But the main idea is that  As for the MultiNodeEdit: 
 In that case, it's pretty difficult to determine how to proceed. My initial idea would be to have godotengine/godot-proposals#13163 implemented so  
 
 That was one of my worries, but currently there's no way to indicate that if not by an icon. Reduz's puts a counter on here, which is a really weird design IMO and not rly descriptive.   
 Yeah it's wasteful currently, removing Resources only to re-add them again, with the exception of the one that changed, but covering all possible cases an Array/Dictionary can change would be pretty painful 😅   
 @AThousandShips is there a way to circumvent this? My previous approach was changing the tooltip text directly after a set of conditions were met, and there was a lot of overlap (e.g A external Resource with Subresources vs an Internal Resources with Subresouces share more of less the same message).  | 
    
| 
           For TTR, see my suggestions: #109458 (comment)  | 
    
| 
           Added the required changes. For MultiNodeEdit i decided to check the Node selection against the Resource counter. If they're equal or if the edited resource is not external, you're not allowed to make them unique. I'd like to implement this proposal after this gets merged (so this PR is not dependent on another). Therefore a more fine-tuned solution for this can flourish. Since I guess, as it stands now, making many Resources unique in bulk like that is pretty niche, specially since it might break user expectations of what this means, the current solution is fine? 
 Damn, seems like there's no easy solution to indicate a resource is unique/linked, I've tried seeing how blender handles ui and well... I'm not sure how to circumvent this without having an icon of sorts. Long term we could refactor the UI so it accepts more buttons or shinks appropriately. Any suggestions are appreciated, but idk if there's something to be done  | 
    
4cff02a    to
    c0f817d      
    Compare
  
    9950939    to
    97d83d9      
    Compare
  
    | 
           I'm so, so glad this PR exists! I've been trying to build a GDscript based prototype around the start of this year (even consulted with the dev chat and passivestar about it) but failed to find the time to bring it forward. The inability to understand which resources were non-unique has been a great pain point for my team, it brought upon us so many hard to understand interactions. Thank you so much! P.s. I don't think it will be very useful at this point, but here is my attempt for reference https://github.com/farfalk/resource-users-addon. There is an attempt to deal with external resources shared among scenes in the whole project, with a hint to implement a resource database in the .godot folder  | 
    
| 
           @farfalk I'm really happy this PR is being of great use! Nerverheless, since counting external Resources is most useful only when you want to look up which Nodes are using it, it seems this PR can currently do without it.  | 
    
e0a842a    to
    8a2280e      
    Compare
  
    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.
Some minor code problems, but otherwise looks fine now.
| if (!make_unique_button->is_disabled()) { | ||
| _edit_menu_cbk((_is_uniqueness_enabled(true) ? OBJ_MENU_MAKE_UNIQUE_RECURSIVE : OBJ_MENU_MAKE_UNIQUE)); | ||
| } | 
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.
This minor, but there is inconsistency between instant right-clicks and left-clicks that take effect on button release.
I think the most feasible way to fix it is to enable Mouse Right in button mask and check if right mouse is pressed in the pressed callback.
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.
Used Input singleton to discern a left click from a right click, so I also changed the button's action mode to ACTION_MODE_BUTTON_PRESS










Attempts to somewhat implement
godotengine/godot-proposals#904 , godotengine/godot-proposals#9603 , godotengine/godot-proposals#10319 (comment)
Feature
Adds an indicator to tells whether a resource is shared. The button's tooltip shows the number of times a Resource was shared, along with whether there are subresources to be made unique:
Left Click on the Icon makes Resources Unique. Right Click - Recursive
Implementation
For this feature, after a lot of trial and error, I settled for reduz's approach of using a
HashMapto track how many times a Resource is referenced by a Node within a Scene.make_uniqueandmake_unique_recursiveis disabled.Note
Duplicating built-in SubResources is also disabled if the 'Parent' Resource wasn't previously made unique. This is because Making a SubResource unique does not matter if the parent isn't also duplicated. See #109458 (comment) (gradient example)
HashMap— since we can't for certain indicate how many of them there are.Outside from that, I would separate this feature into two parts:
SceneTree Operations
In the Scene tree, you can add, delete, duplicate and paste nodes. Each time you do these operations,
update_resource_countcollects built-in non-empty Resources and add them to the HashMap, along with their respective Node. So basically you update the Resource counter each time you do these operations.Inspector Operations
Each time you made a revelvant change to a property that holds a Resource (e.g Made a Resource unique, added a resource to an Array, etc), you update the Resource counter like above, inserting the Resource and its Node in the HashMap.
Note
When setting Subresources in the inspector (if not external), they should have the same number of References as the parent. So if Reference
Ais referenced by 3 Nodes, if you add a SubResourceB, it should also be linked to 3 Nodes.It uses a much "simpler" version of
update_resource_count(although they could be merged but it would make using this API much more cumbersome for the Inspector and SceneTree).update_node_referencecollects the Resources found withing a single property and either removes or adds to the HashMap. That way you're only dealing with a specific property is being set and not touching what's already added.In short, The inspector part was the most tricky, as I did have to fix all the edge cases found when making unique, setting or removing Resources . As of 09/28, it's pretty clean all things considered. Using strictly
update_node_referenceand tweaking some pre existing code made it much cleaner than it previously was.To-Do List
The following is a to-do list of everything that should be dealt with. There are some things left undetermined on how they should work, so I added Waiting for Input right after the task.
MoveEdit: NVM, moving functionality to icons hasnt been well supported by users in previous attempts by other PRsMake UniqueandMake Unique recursivefuncionality to Icons (Waiting on review for input)make_uniqueif there's only copy of the Resource or It's a subresourceHistory mismatch when undo-redoing packed scenes if they're open on the editor. (Waiting on review for input)Seems to have fixed itselfInconsistent Redoerror might happen??Test Project
Added a test project so people interested in this feature can test it. It's a pretty dynamic feature though, so please toy around it and report any weird behavior!
indicator-test.zip