★ wanayoo — archive 1999 https://github.com/paceholder/nodeeditor/pull/236Nouvelle recherche | Portail wanayoo
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

Make NodeDataType to be extendable by open/closed principle #236

Closed

Conversation

@nuustik
Copy link

@nuustik nuustik commented May 17, 2019

This is needed when DataType is not a simple data object,
but a larger and smarter object that should not be copied by value.
Having it as a pointer with virtual operators allows the model to
extend it and control the operations done with it (NodeConnectionInteraction::canConnect, etc),
where TypeConverter is not possible to use.

This is needed when DataType is not a simple data object,
but a larger and smarter object that should not be copied by value.
Having it as a pointer with virtual operators allows the model to
extend it and control the operations done with it (canConnect, etc),
where TypeConverter is not possible to use.
@nuustik nuustik force-pushed the CDPTechnologies:extendable-nodedatatype branch from 66bd5bf to 323eff6 May 17, 2019
@Daguerreo
Copy link

@Daguerreo Daguerreo commented May 28, 2019

Shouldn't DataType represent just the "type" of the data and not data itself? A "larger and smarter object" isn't just a NodeData?

@nuustik
Copy link
Author

@nuustik nuustik commented Jun 6, 2019

Our use case is that one data type needs to accept different other data types and we want to control this from the model. We cannot use TypeConverters and we have thousands of types and creating converters from all the combinations would be horrendous task and would eat a lot of memory in the register. Besides that we actually do not need to convert any data, we just need to control if the connection can be made or not.
If NodeDataType would be a pointer we could then inherit and override the equals operator to not just compare the ids but to compare lists, or id existence in the acceptance list, e.g. It would allow having more control over connection creating.

@Daguerreo
Copy link

@Daguerreo Daguerreo commented Jun 12, 2019

If NodeDataType would be a pointer we could then inherit and override the equals operator to not just compare the ids but to compare lists, or id existence in the acceptance list, e.g. It would allow having more control over connection creating.

you can do it the exact same thing with the current NodeDataType implementation.

@nuustik
Copy link
Author

@nuustik nuustik commented Jun 26, 2019

What do you mean? The NodeConnectionInteraction::canConnect compares IDs of NodeDataType. This is not enough for my use case. I need to override this comparison in my code to make the connections possible. Even if I change NodeEditor to compare structs instead of IDs and add struct comparison functions to NodeEditor I still can't override them in my own code to do smarter comparisons.

@nuustik
Copy link
Author

@nuustik nuustik commented Jun 26, 2019

If NodeConnectionInteraction::canConnect would use NodeData::sameType method instead of comparing the IDs directly, then I can work with it but I guess this is not wanted.

@paceholder
Copy link
Owner

@paceholder paceholder commented Dec 6, 2020 •

I think it could be possible to assign "custom" NodeConnectionInteraction to FlowScene by implementing FlowScene::setCustomNodeConnectionInteraction(NodeConnectionIteraction const & nci).

Then we could ask NodeConnectionInteraction const * FlowScene::hasCustomNodeConnectionInteraction().
If the returned pointer is not nullptr we'd use the overridden function canConnect.

Inside you could use some matching table tailored to your application.

QMap<id, id> compatibleTypes.

By doing all this we'd keep the "can connect" behavior encapsulated into one class and do not introduce plenty of complex changes.

@paceholder paceholder closed this Dec 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.