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.

Sign up for GitHub

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

Jump to bottom

Import behave flow + flow side by side model viewer #159

Closed
wants to merge 8 commits into from
Closed

Import behave flow + flow side by side model viewer #159

wants to merge 8 commits into from

Conversation

oveddan
Copy link
Collaborator

@oveddan oveddan commented Nov 22, 2022 •
edited
Loading

This PR imports the code from @beeglebug's behave-flow into the main behave-graph monorepo, so that the flow code and be developed and updated in sync with the core library code.

A future V2 pr would separate out reusable components and hooks so that developers could import them and integrate them into their existing solution.

It also adds the following improvements, which it incorporates from @oveddan's interX:

Interactive 3d Scene Preview

GLB files can be rendered and interactive in a three.js scene side-by-side with the flow editor, and updates to the editor apply in real-time to the scene.
flowEditorGif

adjustingEditorInRealTime

User can upload their own 3d model:

(todo: gif)

User can load examples that contain both a model and behave graph

...if only behave-graph loaded (without a 3d model), then view will not be split:

loadingScenes

Split pane can be adjusted

A user can drag the split in the middle, and also change the split to be vertical or horizontal:

splitPaneAdjustment

  • react-three-fiber renderer for the 3d scene.
  • new example with button that you press, it starts and elevator animation, and the button turns green
  • a bunch of useful reusable hooks that encapsulates the react-flow functionality
  • an extension of IScene that lets you query properties on a model
  • a way to let the user generate the json path by selecting values from a dropdown

handle

ToDo:

  • Update the main readme with instructions on how to run
  • Deploy this somewhere (netlify?)
  • Allow the user to adjust the environment of the scene stage.
  • Documentation!

@oveddan
Added behave-flow to repo.
2f263f7 
Made editor so that you can toggle between split editor and not, depending on if you have a model file

code cleanup - already have graph json so we just grab it and render it instead of regneerate from nodes and edges

Fixed flow editor with new build and subfolder.

Added a readme in examples
oveddan
oveddan commented Nov 22, 2022
View reviewed changes
.eslintrc Outdated
@@ -30,7 +30,6 @@
"argsIgnorePattern": "^_"
}
],
"import/extensions": ["error", "always", {"ignorePackages": true} ],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bhouston this is kind of a difficult rule to follow, it means you need to add the file extension whenever you import in typescript.

Is it ok to get rid of this rule? Makes the code easier to write/read when you can just do:

import { useRegistry } from './hooks/useRegistry';

wheras before you would need to do:

import { useRegistry } from './hooks/useRegistry.js';

Copy link
Owner

Choose a reason for hiding this comment

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

I did this because it allows for many loaders to load JavaScript files in the browser without bundling. This explains the reasoning: https://stackoverflow.com/questions/70461497/why-do-i-need-to-add-the-js-extension-when-importing-files-in-typescript-but-no?utm_source=feedburner&utm_medium=email

That said, I can just always use a bundler.

On that topic, right now if there are errors in the code, say when I run exec-graph, I do not see any source maps. It makes it a bit harder to track down the errors to the original line in the typescript file.

Copy link
Collaborator Author

@oveddan oveddan Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When would you want to load js files in the browser? I think we could just use the chrome debugger and it loads the typescript files via its sourcemaps?

Yes seeing source maps would be ideal when running exec-graph. A nice way to do this is to just use ts-node - then you get all errors showing up in the original source code location. And you get rid of the need for the step to build the files before. I originally wanted to setup running the exec-graph scripts using ts-node, but it will not work when you want to import .js files:
TypeStrong/ts-node#783

packages/core/src/Profiles/Scene/Events/OnSceneNodeClick.ts
import { Graph } from '../../../Graphs/Graph.js';
import { EventNode } from '../../../Nodes/EventNode.js';
import { NodeDescription } from '../../../Nodes/Registry/NodeDescription.js';
import { Socket } from '../../../Sockets/Socket.js';
import { IScene } from '../Abstractions/IScene.js';

// very 3D specific.
export class OnSceneNodeClick extends EventNode {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bhouston could you please provide feedback on this implementation?

packages/core/src/Profiles/Scene/Abstractions/IScene.ts
@@ -5,4 +5,8 @@ export interface IScene {
jsonPath: string,
callback: (jsonPath: string) => void
): void;
removeOnClickedListener(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added so that it can be unbound on dispose/cleanup

packages/core/src/Profiles/Scene/Events/OnSceneNodeClick.ts

private sendNodeClickedData = (engine: Engine) => {
engine.commitToNewFiber(this, 'flow');
engine.commitToNewFiber(this, 'secondFlow');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ToDo: delete this

packages/core/src/Profiles/Scene/Events/OnSceneNodeClick.ts
[],
[new Socket('flow', 'flow'), new Socket('float', 'nodeIndex')]
[new Socket('string', 'jsonPath')],
[new Socket('flow', 'flow'), new Socket('flow', 'secondFlow')]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ToDo: Delete

@beeglebug
Copy link
Collaborator

This a great start, but I'm not happy merging in the current state.

I've stated elsewhere that I'm only happy for react-flow to become part of the monorepo if it stays an independent npm module, and decoupled from everything except for it's main dependencies, react-flow and behave-graph (so it can be imported elsewhere).

I'm not super happy with it being relegated to example code and the only copy of it being so tightly integrated with a 3D preview.

Could we perhaps tweak this PR to keep the 3D example (which is great btw), but also keep react-flow as a separately developable and deployable library?

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022 •
edited
Loading

@beeglebug that totally makes sense. That was going to be the next thing to do - separate those as their own importable lib. I tried this for a bit but it required modifying the root build config of the monorepo to support building react components, and also ran into challenges with tailwind inside of an imported module. I was going to do the separation all as part of a separate PR in order to do things iteratively but if you think it's better it can be done all at once in a single PR

@beeglebug
Copy link
Collaborator

beeglebug commented Nov 22, 2022 via email

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022 •
edited
Loading

the idea is it would be a standalone package within the packages folder, so there would be:

packages/core (where the current behave-graph core code is, such as nodes engines).
packages/editor (where the reusable behave-flow code would sit). This folder would have its own package.json, so you could still import it in in another app with import {Flow} from '@behave-graph/editor'

examples/editor would have minimal code that just imports the components from packages/editor and could render it side by side with the three.js scene

There are lots of advantages to keeping it in a single mono repo organized into packages, mainly that the react-flow code can be updated in parallel with the core lib code.

This is pretty much the same thing as having it in a separate github repo, except you get the benefit of being able to update and see all in one place.

This type of structure is how tensorflow.js, react-three-fiber, and react are organized

@beeglebug
Copy link
Collaborator

beeglebug commented Nov 22, 2022 via email

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022 •
edited
Loading

yeah sure will do it in one go then. thanks for looking out to make sure things are done the right way!

@beeglebug
Copy link
Collaborator

beeglebug commented Nov 22, 2022 via email

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022

curious if you've seen anywhere examples of how to have tailwind in a module you import, not sure what the right approach is here.

@beeglebug
Copy link
Collaborator

beeglebug commented Nov 22, 2022 via email

@bhouston
Copy link
Owner

This definitely should be another "package. Maybe called it "package/react-flow" and it would be published under the name "@behave-graph/react-flow"?

I think we should have this "react-flow" package separate from the editor if possible. Because some people have non-three.js editors.

@beeglebug
Copy link
Collaborator

beeglebug commented Nov 22, 2022 via email

@bhouston
Copy link
Owner

I'm tempted to try and come up with something less tied to react flow,
behave-flow is it's own thing really, and more tied to behave-graph than
react-flow,

The reason I think that "@behave-graph/react-flow" makes sense to me is that there is already "@behave-graph" in the package name and this is an adaption of React Flow to working with behave-graph. I wouldn't want to repeat the name "behave" again. Just like I call the library "core" now but within the "@behave-graph/core" namespace so it is clear, it is the core engine of the "@behave-graph" set of tools.

Basically because have the upper namespace "@behave-graph", we do not need to fully describe how the sub packages relate to behave-graph, as it is assumed. :)

I've seen this elsewhere as well, like with @react-three. The core library is just "fiber" and it has other similar simple names for the packages but you know they are all for @react-three because it is the upper level namespace.

@bhouston
Copy link
Owner

I'll do a bit of work on this PR now as I have some time. I'll try my hands at moving around behave-flow into the packages section so we can publish it as its own module. We can of course rename it easily.

@bhouston
Copy link
Owner

@beeglebug how about "@behave-graph/flow"? Is that nicer than "@behave-graph/react-flow"?

@bhouston
Copy link
Owner

Damn, the behave-flow package is intermingled with the editor now. I suspect that is probably because its existing interfaces were not sufficient for integrating with a 3D viewer, so @oveddan you had to modify it? I wonder if we can keep those modifications and put it back into a reusable component separately? I am unfortunately not great at React so this is not something I should be leading.

I think we need at least 2 packages:

  • @behave-graph/flow or @behave-graph/react-flow - built using preconstruct like "core"
  • @behave-graph/three, which is the integration into ThreeJS - built using preconstruct like "core"

And then we have an example that uses these two and makes an editor out of these two components. But I would suggest keeping the crypto-elements out of it for now. The more we push into reusable libraries the easier it should be for you to make your crypto-specific extensions on top of this work, rather than having to copy large blocks of code and modify them... it is actually an excellent test of whether we have the right abstractions.

Maybe I can try to pull out the three-specific things? I'll try to do this for a bit longer.

@beeglebug
Copy link
Collaborator

Sorry for any confusion I wasn't suggesting @behave-graph/behave-flow, just saying I don't necessarily want the full "react-flow" library name in there, as to me it implies it's a fork or something, when it's (hopefully) much more than that.

I would also be 100% against any crypto stuff in this repo, its so far removed from the original scope of the project.

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022 •
edited
Loading

Yes agreed the crypto stuff doesn't belong in here and there was no intention to import it - it is not anywhere in the code that's being used...some of it may have been accidentally brought into the lib during a copy and paste; I will clear out any remnants of it.

Agreed with the @behave-graph/flow and @behave-graph/three structure

Damn, the behave-flow package is intermingled with the editor now. I suspect that is probably because its existing interfaces were not sufficient for integrating with a 3D viewer, so @oveddan you had to modify it?
.

Not sure what you are saying here - where was there an editor before, and where was it separate before? I think I may have created confusion when labeling the model viewer an "editor" in this PR description. This just added a model viewer next to the flow editor, and hides the model viewer if there is no model, making the flow editor full screen. The next plan was to extract the reusable behave-flow components into their own lib. We discussed this in the thread above - the plan was originally to have a 2nd pr to do that separation but now we will just do it all as 1 PR

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022 •
edited
Loading

Also there may be some nice reusable react-three-fiber based components built in react, where would they go? I could see an argument for a @behave-graph/react - which would provide hooks that make it easy to imprort behave-graph into a react app with a few lines of code. But I'd also be concerned about having too many packages.

@oveddan oveddan changed the title Import behave flow + flow side by side model editor Import behave flow + flow side by side model viewer Nov 22, 2022
@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022 •
edited
Loading

Accidentally imported crypto-related remnants removed here f6e36a8

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022 •
edited
Loading

also - it may make sense to have 2 separate examples - one with the model viewer side by side with the flow editor, and another with just the flow editor for non 3d-based examples. The way this demo currently works is that if there is a model file, it'll show it side by side, and if not, it just makes the flow editor full screen. Curious your thoughts on having 2 separate examples, one for each case, or having it all as just 1 example like this

oveddan
oveddan commented Nov 22, 2022
View reviewed changes
examples/flow/src/flowEditor/components/LoadModal.tsx
return style;
};

export const LoadModal: FC<LoadModalProps> = ({ open = false, onClose, handleGraphJsonLoaded, setModelFile}) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This component was modified to allow loading a 3d model, as well as preview it, but based on our discussion in the PR it probably makes more sense to have the 3d model loaded somewhere else, as to not clutter the core flow-editor with three.js related things. I can work on reverting that change.

examples/flow/src/App.tsx
/>
</div>
{/* @ts-ignore */}
<GltfLoader fileUrl={modelFile?.dataUri} setGltf={setGltf} />
Copy link
Collaborator Author

@oveddan oveddan Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this needs to be here because we cannot use useGLTF if the input value is possible null - so this is kind of a hacky workaround where you only load the hook if there is a value for the model url. Will look into a better way to to this - like just using the GLTFloader directly instead of the useGLTF hook.

@bhouston
Copy link
Owner

I've just pushed splitting out "@behave-graph/three" from the "three-viewer" example. This is of course different than the react-three module you derived from it.

oveddan
oveddan commented Nov 22, 2022
View reviewed changes
Copy link
Collaborator Author

@oveddan oveddan left a comment

Choose a reason for hiding this comment

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

Awesome! this makes perfect sense.

@bhouston
Copy link
Owner

Also there may be some nice reusable react-three-fiber based components built in react, where would they go? I could see an argument for a @behave-graph/react - which would provide hooks that make it easy to imprort behave-graph into a react app with a few lines of code. But I'd also be concerned about having too many packages.

I think it is fine. I think "@behave-graph/react-three" would be the best place for the Three.js react-three fiber viewer.

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022

Great will start working on this later. Thanks for both of yours feedback!

@beeglebug
Copy link
Collaborator

beeglebug commented Nov 22, 2022 via email

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 22, 2022 •
edited
Loading

I think we need to keep in mind which bits of this are intended to be exported as packages for other people to import, and which bits are just examples built with the underlying packages, to show what the core graph library is capable of. I'm a little wary of expanding the scope of the org/repo to include every possible thing that can be built with behave-graph.

yeah agreed with this. I do see though react-three-fiber as a common way people would want to use this framework, and providing a couple importable hooks or components that let people do this with a few lines of code would be great; it is something I would import in my own projects - but that also could in theory be its own lib.

So my thoughts are for this PR - separate reusable flow things into their own package and import into example. Hold off on react-three-fiber lib extraction for now (keep that stuff in examples) and leave that for a future discussion/PR

@bhouston
Copy link
Owner

So my thoughts are for this PR - separate reusable flow things into their own package and import into example. Hold off on react-three-fiber lib extraction for now (keep that stuff in examples) and leave that for a future discussion/PR

I am okay with that. :)

@beeglebug
Copy link
Collaborator

i've just started going through the code and adding comments, but i've given up because theres too many things to comment on.

i honestly think all the modifications to behave-flow need stripping out before it gets moved to the this repo, almost every change I can see is super specific to the project you used it in, and is not generic enough to remain in place in the published library

at this point I think a fresh PR to add behave-flow into the project as a package would be a better starting point, then we can come back and look at how we allow you to bolt extra functionality on top of the library to enable this complex example in a more composable way

@beeglebug beeglebug self-requested a review November 23, 2022 13:26
@bhouston
Copy link
Owner

i honestly think all the modifications to behave-flow need stripping out before it gets moved to the this repo, almost every change I can see is super specific to the project you used it in, and is not generic enough to remain in place in the published library
at this point I think a fresh PR to add behave-flow into the project as a package would be a better starting point, then we can come back and look at how we allow you to bolt extra functionality on top of the library to enable this complex example in a more composable way

I noticed that as well.... #159 (comment)

Okay. I can do that.

@bhouston bhouston closed this Nov 23, 2022
@oveddan
Copy link
Collaborator Author

oveddan commented Nov 23, 2022 •
edited
Loading

In my PR I was actually working on extracting out the 3d specific things to be only in the example, and leaving the core behave-flow library being generic, and had that done (just hadn't pushed it yet). I've just pushed that update that extracts the side-by-side functionality into examples and leaves behave-flow being generic into my branch, but it doesn't show up here since the PR has been closed - you can see the standalone package here

The only thing I got hung up on, which whoever will take on the new PR would deal with as well, would be how to import the css that is bundled, given it depends on postcss and taliwind. It's not so straightforward and theres little documentation on how to do so because it's not typical - in fact I would suggest removing tailwind from behave-flow in this repo, and using css modules so that the styles can be scoped to the components that are imported and not imported globally. Or if the desire is to use tailwind, you may have success using something like this - I tried but had no luck.

almost every change I can see is super specific to the project you used it in, and is not generic enough to remain in place in the published library

I'd disagree with this characterization - there are many improvements to in this PR to the core behave-flow:

  • extracting reusable functionality into hooks that could become an API (you can see then all extracted here)
  • improving performance by wrapping functions in useCallback.
  • Have the graph and engine auto-update when playing if any node is changed (in the useEngine hook)

That being said - I agree that the better approach, as you suggested, would have been to import behave-flow as is, then add these improvements after the fact, as well as making its api more flexible to allow you to build on top of it. One thing that would be great would be being able to define custom renderings for input or output nodes.

I also think a lot of the react functionality from this work would be useful in a standalone way (@behave-graph/react ?), in particular:

@beeglebug
Copy link
Collaborator

beeglebug commented Nov 23, 2022 via email

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 23, 2022 •
edited
Loading

I'll definitely like to keep some of the hooks, although from a quick look a lot of the usage of useCallback needs double checking, i can see a lot of missing dependencies which can cause subtle bugs. They're all very easily fixed though, so I would definitely consider porting slightly modified versions of the hooks into the new PR.

yeah good eye - the missing dependencies in the hooks are a bug. would be good to turn on the eslint rules around that.

@beeglebug
Copy link
Collaborator

beeglebug commented Nov 23, 2022 via email

@oveddan
Copy link
Collaborator Author

oveddan commented Nov 23, 2022 •
edited
Loading

I also cannot imagine a life without tailwind! But I think you could still use it the way it is used now in behave-flow. You would just need to make sure that whoever is importing the behave-flow components into their project also imports tailwind.

The only thing that would be good to change are these two css imports:
https://github.com/beeglebug/behave-flow/blob/main/src/index.tsx#L5

I would say the index.css rules could be done in a css module so they are scoped to the component, and you could require whoever is importing the lib to import reactflow/dist/style.css into their app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhouston bhouston bhouston left review comments

@beeglebug beeglebug Awaiting requested review from beeglebug

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oveddan @beeglebug @bhouston