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

🛠️Execution Refactor & 🎉 Nested Nodes #113

Merged
merged 27 commits into from
Dec 20, 2021
Merged

Conversation

vanyle
Copy link
Contributor

@vanyle vanyle commented Dec 11, 2021

A lot of new blocks have been added but they are not executable yet !

This pull request aims to:

  • Make the new blocks like slider executable
  • Polish the executing code
  • Fix some bugs
  • Introduce new node types

While this might seem like multiple features added at once, because they are all deeply intertwined, it makes sense to group them in the same branch.
More specifically, we will try to fix:

…uted. This will be used to make sliders "executable".
@vanyle vanyle force-pushed the feature/nested_node branch from e3ae908 to ee02258 Compare December 11, 2021 20:44
@vanyle vanyle force-pushed the feature/nested_node branch from 256c948 to e9c2615 Compare December 11, 2021 22:47
@vanyle vanyle force-pushed the feature/nested_node branch from 7bbaa50 to 17fef8a Compare December 11, 2021 23:13
@vanyle
Copy link
Contributor Author

vanyle commented Dec 11, 2021

By creating a new kernel when creating a new scene, file loading and file creation is a bit slower.
This could be fix by making the creation of the kernel asynchronous, but this is another issue that is less critical.

So in case somebody in the future wants to increase file opening performance, he should focus on this.

@vanyle vanyle force-pushed the feature/nested_node branch from e1e92c7 to b1762ce Compare December 12, 2021 00:08
@vanyle vanyle marked this pull request as ready for review December 12, 2021 00:30
@vanyle
Copy link
Contributor Author

vanyle commented Dec 12, 2021

A lot has been done. Nodes can be grouped but nodes inside containers cannot interact with the outside. They don't even share the same kernel ! (And I'd argue this is a good thing as containers might run on different python versions)

…e of the developer that might have been better spent implementing useful features or resolving bugs)
Copy link
Member

@MathisFederico MathisFederico left a comment

Choose a reason for hiding this comment

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

There is some things that we will need to change:

  1. Allow to change slider min, max and step, and base scale (linear vs exp?)
    image

  2. Make nested blocks easier to use.

  3. Make output panels always open when raising error, color block when ran / failed.

But those will be for other PRs.
From what has been announced, this PR still lacks of multiple tests.

opencodeblocks/blocks/containerblock.py Outdated Show resolved Hide resolved
opencodeblocks/blocks/executableblock.py Show resolved Hide resolved
opencodeblocks/graphics/pyeditor.py Show resolved Hide resolved
@vanyle
Copy link
Contributor Author

vanyle commented Dec 12, 2021

Working on adding the tests right now !

MathisFederico
MathisFederico previously approved these changes Dec 20, 2021
Copy link
Member

@MathisFederico MathisFederico left a comment

Choose a reason for hiding this comment

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

I trust you, I've not had time to review everything as there is too much changes in this PR

@vanyle vanyle force-pushed the feature/nested_node branch from 54283f9 to 41b7f17 Compare December 20, 2021 15:54
…manager. These are not integration tests ! There is no QApplication setup that allows you to retrieve theme information.
@vanyle vanyle force-pushed the feature/nested_node branch from 41b7f17 to 2817584 Compare December 20, 2021 15:59
@AlexandreSajus AlexandreSajus self-requested a review December 20, 2021 16:40
Copy link
Contributor

@AlexandreSajus AlexandreSajus left a comment

Choose a reason for hiding this comment

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

Looks good to me. run_left and run_right will be modified further in feature/visual-flow

@vanyle vanyle merged commit 9ab9d32 into master Dec 20, 2021
@vanyle
Copy link
Contributor Author

vanyle commented Dec 20, 2021

Don't delete the branch ! Nested blocks need to be improved

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.

The chaining blocks, some blocks are executed twice. Kernel CWD is incorrect
3 participants