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

[Explanation] This node #21

Closed
gregsn opened this issue Jun 22, 2020 · 3 comments
Closed

[Explanation] This node #21

gregsn opened this issue Jun 22, 2020 · 3 comments

Comments

@gregsn
Copy link
Member

gregsn commented Jun 22, 2020

There has been a discussion in the vvvv.org forums: https://discourse.vvvv.org/t/this-pin/18127

We were embracing the idea of having This modelled as an input pin called Input or State Input with the idea that later on Output or State Output might get added.

We finally didn't go for that idea but implemented a This process node.

Here is why:

  • We were unsure about the naming Input or State Input would be nice, but which one is it?
    • Input doesn't work out of the box. You'd need to flag it somehow to make sure that the system understands that you actually want to access the incoming state.
    • State Input might be confusing since on application side it is just called Input
  • We figured out that we will never need a way to feed the State Output/Output (seen on application side)

We never need to feed the State Output

Let's start with a
Record
You define an operation on your record and want to output a new snapshot. Here is what you'd do:

  • Either just write into pads
  • Or call an operation on your own instance and feed that to the outside. How? There is just one way to do this: feed the snapshot returned by MyOtherOp explicitly to an output pin. You can name that output as you like. Let's say you call the output Output: you already would get exactly what was asked for. Your own definition returns a new snapshot on a pin called Output. (There is no implicit state output pin, since you don't write into pads yourself)
  • If you'd do both at the same time you'd get two outputs reflecting that one instance got built by writing into the pads and one output would come with the snapshot that got constructed by the other call. This would be a weird node probably, but it feels straight-forward.
    We compared this approach to other ideas where we tried to merge the changes instructed via pads and instructed via calls on yourself. But that got pretty messy resulting in a snapshot that got frozen after leaving the outer patch, but being mutable up to that point. It didn't feel like a reasonable idea to follow.

So we felt like there is no need to add some new idea to get a new snapshot outof a record instance operation. No matter whether you write the pads on your own or delegate that task to another operation...

Class
When changing fields inside a mutable data type you need to reason about what to do in what order. #19. But one way or the other: You are just mutating yourself. This is reflected on application side: the state Input and Output pins are connected.
This visually says: The output is always the same instance as the ingoing one. It would be plain weird if you'd be abe to overrule that within the operation and just write something else into the state output. Add another output pin if you want to create a new instance manually and hand that over via an explicit new pin.

So all in all. We don't need a way to write into the State Output.

What does that mean for the Input

So we had issues with the right name for that input. We could have called it This, but there is no pin on application side with that name...

Decision

We went for a process node This that always outputs the incoming instance. It helps explaining how to translate C# code into a patch and feels lightweight. It is modeled as a process node as it only makes sense in stateful context

@azeno
Copy link
Member

azeno commented Jun 15, 2021

Little follow up after looking into the topic of making This available inside the Create operation:

In order to have This available inside of Create the following steps are required:

  1. Create the instance using a default constructor. Its fields are not intialized yet.
  2. Call the private __Create__ method on that newly created instance. The __Create__ method will map to the user defined Create operation. At the end it will initialize all fields with the values as definend in the patch.
  3. Return the instance

These changes are now available in VL >= 2021.4-0127

Note that the fields will be written directly and only after __Create__ are properly initialized. This means that calling any other operation during that call will have undefined behavior because either

  1. the fields they're accessing are not initialized and will most likely result in a null pointer
  2. or the values written to the fields will get lost since __Create__ will write them again at the very end

So currently we can't use the This node to call another operation like Reset during Create.

Two ideas to solve this issue

  1. Introduce a new built-in operation OnInitialized
    This new operation would be called by the system right after Create. It would not be visible from the outside to ensure it's only called once.
  2. Modify the execution order of Create
    1. Initialize all user defined fields with defaults first. This ensures that the values written by another operation don't get lost (at least for the mutable case) as well as fields accessed by another operation are initialized.
    2. Ensure that the Create fragments of all process nodes are traversed before the This node. Ignore those which have been traversed already. This ensures that process nodes are initialized before they get accessed by a call to another operation (at least as long as there's no cycle which would lead to a null pointer).

@gregsn
Copy link
Member Author

gregsn commented Jun 17, 2021

@azeno and I have discussed this internally in detail and I thought it would make sense to also write down the ideas as this was a rather long discussion.

Ok so here is what we touched:

  • We want This to be well defined on Create or
  • don't allow it and rather introduce Initialize.

My intuition here was that introducing Initialize would be more of a pattern forced upon the user and thus not as versatile as just figuring out how Create and This work well together. But then again: if many user patches turn out to actually make use of the pattern explicitly by calling Initialize on Create manually (by connecting an Initialize-call to a This node), then maybe that pattern can still be introduced later on?

Not sure. And not sure about the order how to tackle the issue. Still, let's see which questions would need to be solved first, before being able to decide which one is the better solution:

So let's assume the user calls Initialize on Create on the own instance (This) manually. What are the open questions when defining how This shall operate on Create?

  • the user makes use of a process node FlipFlop. The Reset fragment gets called on Initialize. So the process node has to be initialized before we call Initialize by calling the Create-fragment of the FlipFlop.

  • Is it always possible to call the Create-fragments of all process nodes before calling methods on This? What if the Create fragments come white input pins and we want to feed with a value that only is accessible after calling MyOtherOp on This? Do we want to get an error in that case or shall that also work as long as MyOtherOp is not operating on the process node as well? Are these cases too rare to be discussed in this detail?

  • the user additionally writes into a pad A on Create. Is the field A written before or after calling Initialize. If we'd access the field on Initialize, what value would it have? The .Net default value (like NULL), a proper VL default value, or the value that got fed into the setter on Create?

  • What about all the fields that we don't touch in Create? Will those be written as early as possible (before calling any user code)? Can we just do this for all fields so that we have some defined fallback behavior?

How is the answer to these questions connected to other quests, like the one regarding execution order #15 and the other one regarding through-pads #19? Is it possible and clever to try to get a better understanding of those issues first, before tackling these special cases?


A possible way we could go:

By this, we'd have quite some issues out of the way already. The user could express manually when to write fields, so before or after a recursive call. It feels nice to have a working manual idea, that already can be used if the to be invented magic doesn't work as expected.

The magic sauce could be:

  • If there is a This-node on Create in a class. Write all fields with VL default values, before doing anything else.

  • Try to call all Create-fragments of all process nodes before calling This, if this doesn't contradict the dataflow and execution order statements by the user. Don't complain.

  • If the user writes into a field manually by using a pad, this is done as late as possible. If you are not happy and want to initialize a field before calling Initialize: use a way to express execution order. Todo: discuss options of [Proposal] Classes read and write fields on the fly, "through-pad"s get a warning in Records and Process Patches #42 This actually already doesn't belong here

@azeno
Copy link
Member

azeno commented Jul 5, 2021

The magic sauce is now implemented in 2021.4. The core ideas of this issue have been tackled.

@azeno azeno closed this as completed Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants