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

Iterator Rewrite: Part 1 #2254

Merged
merged 59 commits into from Oct 14, 2023
Merged

Iterator Rewrite: Part 1 #2254

merged 59 commits into from Oct 14, 2023

Conversation

joeyballentine
Copy link
Member

@joeyballentine joeyballentine commented Oct 12, 2023

Part 1 of the Iterator Rewrite ™️. While not everything we want to be able to accomplish out of iterators, it works so much better than the current system.

Improvements:

  • No more giant sub-editor behemoth nodes getting in your way
  • Iterators and collectors can now be mixed and matched, for example to convert an image sequence to video
  • Visual indicator on handles when something is iterable or collects an iterable sequence

Drawbacks:

  • While you can mix and match iterators with collectors, you can only use one iterator per "lineage"
  • no explicit mapping operations, or fancy things like take/every or filtering
  • No type system information for iterators
  • Still doesn't allow nodes to expand or contract the amount of items in an iterable, so frame interpolation is still not viable

Open Questions:

  • Are the names of the nodes good?

Things that should be extensively tested:

  • Regression of functionality. These should be identical to existing iterators
  • Migration of old iterators to new
  • The video iterator is a bit different, so we need to be sure I didn't break anything

Future work:

  • There's a lot of old iterator code that I left in. I kinda don't wanna bog this PR down with removing all that, so I plan on doing that in a separate PR
  • Expanding this system to fix the drawbacks

Issues that get closed as a result of this:

image

@RunDevelopment
Copy link
Member

Found some bugs.

  1. You cannot connect to an already-connected input that is downstream of an iterator. Example:
    image

  2. Non-iterator outputs can be connected to iterator inputs.
    image
    image

@joeyballentine
Copy link
Member Author

Non-iterator outputs can be connected to iterator inputs.

Without type information I dont think that can be avoided. I could just automatically convert those to arrays I guess?

@RunDevelopment
Copy link
Member

I could just automatically convert those to arrays I guess?

Let's not make a bug a feature, okay? Since we should make iterators a type in the type system anyway, we can fix it in later PRs I guess.

On that note: we should make issues for the iterator things we want to change/fix.

backend/src/process.py Outdated Show resolved Hide resolved
backend/src/process.py Outdated Show resolved Hide resolved
backend/src/process.py Outdated Show resolved Hide resolved
src/renderer/contexts/GlobalNodeState.tsx Outdated Show resolved Hide resolved
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; We still need to polish iterators a lot, but this is a solid foundation. Let's merge it!

@joeyballentine joeyballentine merged commit b951110 into main Oct 14, 2023
17 checks passed
@joeyballentine joeyballentine deleted the iterator-rewrite-attempt branch October 14, 2023 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment