Simplify Interpolators class

TheYellowArchitect commented 2 months ago

Having written the class value-to-bytes which does something similar (returns different value type depending on the type), Interpolators is overengineered, as it could be a switch-case static function.

With each property entry containing its type (see #251 or #259), you don't need any typeof in realtime by simplifying it. Without the new variable property_entry.type Interpolators would be more performant, but with this, there is no reason.

Assuming the above PR is merged, I would even suggest the tag "Good first issue" for this issue

annamaria-szentpeteri commented 2 months ago

value-to-bytes can only work with built in types, no support for user defined types. My understanding is that user defined types should be supported by serialization in netfox. #251 should support this as well.

TheYellowArchitect commented 2 months ago

value-to-bytes is unrelated to interpolators, i only brought it up because it made me reflect on how Interpolators should work. That said, value-to-bytes works with Godot's variant types, as it should be. You say "user defined types", you mean custom classes. You cannot RPC custom classes (Godot engine limitation), so there is no reason for #251's serializer to support serializing custom classes (netfox doesn't support that either, it simply extracts the properties of custom classes, and this serializer serializes these variables of custom classes)

It's very easy to serialize your own custom classes anyway, if you post an example I can write a serializer exclusive to your project lol The serializer of #251 is meant for netfox's properties which is about Godot variants.

annamaria-szentpeteri commented 2 months ago

You say "user defined types", you mean custom classes.

Types/Classes that are defined by the users of Netfox. :) You know in this case these are synonyms, right?

About the issue: as you said, value-to-bytes has nothing to do with Interpolators, so your reasoning for this issue is flawed. Just because value-to-bytes is supposed to be okay as a switch-case, doesn't mean Interpolators is over-engineered. Maybe you wanted to add "in my opinion" to your text?

TheYellowArchitect commented 2 months ago

Everything I say is "in my opinion", i am not objective, no one is. And yes, nested lambda functions which you convert to function pointers (aka callables) then cache in dictionaries (at tick-interpolator.gd) for a simple switch case, is overengineering.

I don't understand why you derail this issue to off-topic with value-to-bytes, without even offering a code example of a custom class for value-to-bytes, neither here, nor in the PR. Yet you claim "it doesn't work for custom classes", even though normal netfox doesn't send custom classes over RPCs.

Post a code example of a custom class for serialization, or custom interpolator for the new interpolator - a real argument - instead of saying "omg interpolators is not overengineered"

annamaria-szentpeteri commented 2 months ago

This issue is about Interpolators class, is it not? That's why I was not reacting to value-to-bytes and rpc. Those have nothing to do with Interpolators. How is talking about Interpolators is derailing the issue?

As I pointed out in the PR, you removed features from Interpolators class because you deemed it over-engineered.

Very basic code example of overwriting the built-in interpolation for Vector2 would be:

Interpolators.register(
    func(a): return a is Vector2,
    func(a: Vector2, b: Vector2, c: Vector2, d: Vector2, f: float): return _cubic_bezier(a, b, c, d, f)
)

TheYellowArchitect commented 2 months ago

That cubic bezier argument is actually valid. I thought you were trolling lol

It seems adding different lerp for any type is the only functionality removed, I will convert the PR to draft until I think how to solve it, thank you for bringing it up.

annamaria-szentpeteri commented 2 months ago

I thought you were trolling lol

Why you would think that? That's why you were kind of dismissive? Not cool man... :/

TheYellowArchitect commented 2 months ago

I apologize. I thought you were trolling because you started with value-to-bytes in an issue unrelated to that (first 2 replies here), then moved to the PR with a generic reply, so I thought you were trolling. Anyway, at least in the end you found the flaw in the PR, thank you for that :+1:

annamaria-szentpeteri commented 2 months ago

Hm, I was under the assumption that saying "this and this feature got removed with the refactor" would be self-explanatory, even without a code example. Also value-to-bytes was part of the initial issue description, so that's why I mentioned it, but it derailed the start of the conversation.

I think communication should be improved especially because in writing it is easier to misunderstand tone. Like how your style of writing seems like "this is a fact" when in reality you meant it as an opinion, and how it seemed to you that I was trolling.

But this is a bit off-topic, more like a retrospective now. :D

elementbound commented 2 months ago

@annamaria-szentpeteri @TheYellowArchitect I see you both had quite a convo here :smile: I'll keep my notes to this post only.

Interpolators is overengineered, as it could be a switch-case static function.

It is not, and could not be, as pointed out by @annamaria-szentpeteri multiple times. I'm happy to see you both have discussed this. What I'm less happy about is the class being called overengineered multiple times without understanding the underlying requirements it fulfills, or how it's used.

Please be more open to notes in the future @TheYellowArchitect. Thanks!

I thought you were trolling lol

Please engage in good faith instead of assuming malice. Especially since in this case it was you not being clear on the points made by @annamaria-szentpeteri, @TheYellowArchitect.

Everything I say is "in my opinion", i am not objective, no one is.

And this is the internet, where we communicate in text, without non-verbal cues like body language, tone of voice or even just the rhythm of speech, and on top of that, most of us don't know each other very much. So it's very difficult to pick up on subtleties.

The above is important because we talk both about subjective and objective things. So there must be a way to discern what is presented as subjective ( e.g. opinions, intuitions, etc. ), and what's presented as objective ( facts ). To be fair, I've also read most of your posts as posting something objective, even if it's - I assume - more of an opinion, e.g. a class being overengineered. Again, without having a clear understanding of the documented features it provides.

( from the PR )

I haven't stumbled into that kind of lambda function (lambda functions as parameters), so new users will definitely not use it either.

It's literally in the docs with an example. New users generally wouldn't need custom interpolators, but if they do, they can use the example provided.

Okay, you can stop right there.

no one stops you lol Though it does say a lot that you haven't posted a code example of an interpolator you overwrote or added

PRs are not for this kind of behaviour. Please take a look at how you communicate with others.

Seconding this. Please engage in good faith.

To make the above lerp overwrite in this PR, you just replace this line [...] with this [...]

For now I don't intend asking the users of the library to modify the source code. It would mean they'd have to cherry-pick their changes whenever they update their version of netfox, which is extra friction I'd like to avoid.

EDIT: In case you haven't encountered the SOLID principles yet, I'd recommend considering the open-closed principle here.

I apologize.

I greatly appreciate this, @TheYellowArchitect

Overall, this is the internet, it's difficult to pick up nuances in communication, and all of us are very different, so it takes some effort from all of us to interpret what the other wants to communicate. I intend on keeping this a friendly and productive place as much as possible. To that end, please:

Engage in good faith
Be open to other people's views / notes / comments before forming an opinion or response
If possible, be familiar with the feature you want to improve before posting
...or be open to notes about why that feature works the way it does :slightly_smiling_face: I don't expect everyone to fully familiarize themselves with every little detail of netfox.

I want to highlight that my intent is not any kind of discouragement - on the contrary! I greatly appreciate all the effort and care that goes into these PR's and issues. But I want to make sure that these come with productive discussions, and that we are aligned on what netfox is and is not. I would hate to have any of your work be thrown away because of miscommunication.

Edit: I also realize that none of these were documented, these kind of stem from general internet etiquette. I will be adding a code of conduct so that we all can be on the same page.

foxssake / netfox

Simplify Interpolators class #257