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

Refactor storage #48

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

kalemas
Copy link
Contributor

@kalemas kalemas commented Jul 5, 2022

Hi @luckylyk, here is a draft for what we told at #44, i will still thinking about details, but i got it working (on boxes) and if you are interested you may look at.

  • implement removing data
  • load testing
  • get native namespace for picker targets

@luckylyk
Copy link
Member

luckylyk commented Jul 6, 2022

Hey ! That looks nice at first stare.
Unfortunately I'm over busy this week. I will do my best to look deeper and test it and give more feedback ASAP.
Thanks for the involvment anyway !

@kalemas
Copy link
Contributor Author

kalemas commented Jul 6, 2022

Please, no rush, i will think of details more, and i have to do a load testing.
Also i think it should be possible to make higher level storage class that will use both default and my new dag storage under the hood, so user may choose the storage for picker data that will be used when scene referenced, and changing picker data should not result in changing storage type.

@kalemas kalemas force-pushed the refator-storage branch 3 times, most recently from 32b6a74 to 3b3a986 Compare July 6, 2022 20:50
@kalemas
Copy link
Contributor Author

kalemas commented Jul 6, 2022

data = timeit(dwpicker.scenedata.DefaultSceneStorage().load)()
Time for DefaultSceneStorage.load : 0.0050023999938275665
data = timeit(SceneDagStorage().load)()
Time for SceneDagStorage.load : 0.0970897000079276

so it is about 20 times difference, it is fine for me at this stage (200x would be something sad), as i will profile loading in case it may be optimized.

@luckylyk
Copy link
Member

Hi Konstantin,
Sorry for the long delay (vacation, covid and studio emergencies :) )
I checked out youre branch for test. Globally, (even if I always have picky stuff to say about code) I agree with the code design. Unfortunately my usage test is not really concluding (performance issue). The loading time is ok, but the storage data update is terribly slow (severale seconds for each edits). As more buttons the picker has, slower it is. Reaching 50 buttons, you start to feel the latency. With two picker of 100 shapes openend, it is freaking hell to naviguate. (I can provide some heavy pickers if you need test)
May using and array-multi attribute on the picker node for shapes, instead of creating a node for each button would improve performance ?

Less important (as far as it is still WIP), data are easily out for sync:
- Close a tab does not remove the picker node.
- Remove the picker node from outliner does not close the picker tab.
- Remove shape node does not remove the
- Change a shape attribute from maya is not updating picker
- ...
tell me what you think

@luckylyk
Copy link
Member

FYI: i tested on both Maya versions: 2020.3 Py2, 2022.3 Py3 (Win10)

@kalemas
Copy link
Contributor Author

kalemas commented Sep 29, 2022

Hi, sorry for late reply. Yes this was first attempt to get desired data storage, i see several ways to optimize this. First one is to calculate json diff and store it in animation scene. This way an animator would 1. override referenced picker data. e.g. move button group or change color, add his own buttons 2. remove data from scene to reset picker data. 3. it have to be faster to save and load as there will no many api calls. I think api calls are the bottleneck, raw python operations (comparing dicts, serialize/deserialize) are always fast. I have no clue when i will have time to get it.

@luckylyk
Copy link
Member

Hi,
Sorry too for the delay. My reply delay is quite terrible for the moment :|
Yeah give a shot when you have time on a json's delta system, that sounds like a wise solution for performance.
Indeed too much api call can be a nightmare. But especially with so many nodes. May have one node by picker with attributes would be faster. I don't know.

Hope you are safe and well.
Take youre time
Lio

@kalemas
Copy link
Contributor Author

kalemas commented Oct 2, 2023

I ve pushed another idea with separate node for each picker, so they would be easily exported and imported and picker node's namespace used for picker data so nodes would be immediately selected after referencing scene with pickers.

On editing picker will store source picker node name so only edited version loaded. It looks as follows:

image

Not sure if it is best setup, i will think about this more.

What i realize is that we do not need actually deep difference as it should not work with expected behavior. Detaching full picker is fine. It should only have option to revert to referenced state.

@kalemas
Copy link
Contributor Author

kalemas commented Oct 11, 2023

So @luckylyk i still thinking on empower DwPicker in maya referencing pipelines.

  1. i think that no need to implement merging local and referenced data (what you called json's delta system), from user's point of view updates should lead to unexpected behavior.
  2. your approach with copy and cleanup referenced data is quite fine, but i think would be better to leave referenced data unchanged so picker would revert its data to referenced state. I would suggest to do connections between local and referenced picker nodes, presence of any connection would mean that data should not load from this picker node.
  3. We should consider maya referencing specifics like unloading reference should also hide picker tab, renaming namespace of reference is quite rare case but it would also be handled with connections (not sure we should do this).
  4. Should we add something in Ui, may be chain/link icon for referenced pickers? I think it is not necessary, but option in menu to re-load referenced pickers would be fine (so pickers would be merged by hands, what if there is a lot of pickers).
  5. Namespace propagation? Need we dynamically resolve namespace for targets in shape data or just add namespace from picker node to shape targets on loading? I m incline to second.

So my suggestion is to change the way you cleanup referenced pickers and to add option to reload referenced pickers. What do you think?

@kalemas
Copy link
Contributor Author

kalemas commented Oct 11, 2023

  1. There was idea to separate single picker data to single node, so connection between those nodes would mean to load only that is without a history. This will resolve cases when new picker was added in referenced scene. I like this idea but have no good solution for name of picker node, should it be random or generated from picker title. How to handle removing of that picker when it is referenced?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants