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

Models can be added within models #139

Merged
merged 9 commits into from
Jan 12, 2021
Merged

Models can be added within models #139

merged 9 commits into from
Jan 12, 2021

Conversation

gidonkatten
Copy link
Contributor

Can add other models within model editor.

The current model editor is filtered out on the front end to avoid that recursive case although there is no general cycle checking implemented.

Also re-added input connections for model nodes. This impacts the overview editor so @KacperKazan there may be some slight changes needed.

Copy link
Contributor

@tomproberts tomproberts left a comment

Choose a reason for hiding this comment

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

A lot of great stuff! Just my comment about the message, and one other issue: If you rename a Model, the name of the corresponding node will update in the Overview editor, but not in any Model editor which contains that Modek. E.g. there are 2 models: Model1, and Model2. Model2 contains Model1. The name of Model1 changes to newModelName, but if you look in Model2 it still says Model1 not newModelName...

src/components/tabs/LayersTab.vue Outdated Show resolved Hide resolved
@gidonkatten
Copy link
Contributor Author

A lot of great stuff! Just my comment about the message, and one other issue: If you rename a Model, the name of the corresponding node will update in the Overview editor, but not in any Model editor which contains that Modek. E.g. there are 2 models: Model1, and Model2. Model2 contains Model1. The name of Model1 changes to newModelName, but if you look in Model2 it still says Model1 not newModelName...

Also deleting editors has same problem

tomproberts
tomproberts previously approved these changes Jan 11, 2021
Copy link
Contributor

@tomproberts tomproberts left a comment

Choose a reason for hiding this comment

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

All looks good to me, idk if anybody else wants to take a look but I'm happy to approve.

tomproberts
tomproberts previously approved these changes Jan 11, 2021
Copy link
Contributor

@tomproberts tomproberts left a comment

Choose a reason for hiding this comment

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

All seems good to me, tho a bit hacky obviously but I don't know what the long term plans are for model nodes and stuff.

@KacperKazan KacperKazan merged commit d32c5e2 into master Jan 12, 2021
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.

3 participants