NodeGraphProcessor icon indicating copy to clipboard operation
NodeGraphProcessor copied to clipboard

Simple custom conversion (int -> float) fails

Open thebne opened this issue 2 years ago • 5 comments

When adding this to CustomConversion.cs:

public static float ConvertIntToFloat(int from) => from;

I get this:

Can't convert from System.Single to System.Int32, you must specify a custom port function (i.e CustomPortInput or CustomPortOutput) for non-implicit convertions

I believe the problem is here (NodePort.cs) - it should be (output, input) instead - but not 100% sure:

				if (!BaseGraph.TypesAreConnectable(inputField.FieldType, outputField.FieldType))
				{
					Debug.LogError("Can't convert from " + inputField.FieldType + " to " + outputField.FieldType + ", you must specify a custom port function (i.e CustomPortInput or CustomPortOutput) for non-implicit convertions");
					return null;
				}

adding this fixes the problem (however this isn't what I intended): public static int ConvertFloatToInt(float from) => (int)from;

EDIT I tried to solve it by simply switching input<->output (which I think makes sense), but it causes some other issues later on. Why does the conversion need to be symmetrical? It makes sense to me to be able to convert an integer output to a float input, but not the other way around.

thebne avatar Jul 28 '21 00:07 thebne

Conversion needs to be symmetrical because most of the time the type in the conversion can be used in both input and output ports (there is no restriction) and in that case, a one-way conversion would fail.

If you don't want to have a conversion on one side, you have to design your nodes in a way that restrain the conversion to this side only.

Also, for me, it makes sense to have both conversions from int to float, the end-user just has to be aware that each conversion (float to int or int to float) results in a precision loss.

alelievr avatar Jul 29 '21 21:07 alelievr

If you don't want to have a conversion on one side, you have to design your nodes in a way that restrain the conversion to this side only.

Is that a technical caveat or a design choice? I would override the default behaviour you are aiming for, but I found out it is pretty much embedded in the core logic (symmetry enforcement). Wouldn't it be more robust if there could be an option to remove this restriction?

Also, for me, it makes sense to have both conversions from int to float, the end-user just has to be aware that each conversion (float to int or int to float) results in a precision loss.

Unfortunately my end-user is not very technical, and I would be happy to keep them from making mistakes (when talking about float -> int conversion, I bet most people would assume it actually rounds to nearest int).

By the way, thank you for writing this much-needed library 🙏

thebne avatar Jul 29 '21 22:07 thebne

that a technical caveat or a design choice? It was a design choice I made when creating the package. I'll see what I can do to remove the restriction but it may take a bit of time.

alelievr avatar Aug 12 '21 11:08 alelievr

Want to bump this issue. @thebne

I tried to solve it by simply switching input<->output (which I think makes sense), but it causes some other issues later on.

I'm curious what the issues you ran into later were. I altogether removed the check in the snippet from your original post. It seemed somewhat unnecessary. (Also, removed the check in TypeAdapter.cs that was trying to enforce symmetrical conversions)

Additionally, I made this change in BaseGraphView.GetCompatiblePorts

//CUSTOM CHANGE - fixes check regardless of which port we started dragging the edge from
var inType = startPort.direction == Direction.Input ? startPort.portType : p.portType;
var outType = startPort.direction == Direction.Input ? p.portType : startPort.portType;
if (!BaseGraph.TypesAreConnectable(outType, inType))
	return false;

//DEFAULT PACKAGE IMPLEMENTATION
//if (!BaseGraph.TypesAreConnectable(startPort.portType, p.portType))
//	return false;

I've only done some minor tests, but so far everything seems to be working as I would expect.

RandyPasion avatar Dec 08 '21 16:12 RandyPasion

@RandyPasion Frankly it was a while ago and I don't remember. I tried your version now and it looks fine. I use the library extensively on a daily basis so I will test it out for a while and see if it actually breaks anything.

thebne avatar Dec 15 '21 12:12 thebne