Skip to content

[graph] Disallow input dependencies without registered update#52

Open
arntanguy wants to merge 1 commit intojrl-umi3218:masterfrom
arntanguy:topic/RegisterUpdates
Open

[graph] Disallow input dependencies without registered update#52
arntanguy wants to merge 1 commit intojrl-umi3218:masterfrom
arntanguy:topic/RegisterUpdates

Conversation

@arntanguy
Copy link
Copy Markdown
Contributor

@arntanguy arntanguy commented Apr 1, 2025

This PR makes sure that we have a registered update function corresponding to an input dependency.
The reasoning is to avoid a potentially misleading edge case when inheriting from a function and declaring updates.

struct BaseFun : public Node
{
SET_UPDATES(BaseFun, Value)

BaseFun() {
  registerUpdates(Update::Value, &BaseFun::updateValue_);
  addInputDependency<BaseFun>(Update::Value, source, Output);
}
};

Currently it is possible to do

struct DerivedFun : public BaseFun
{
SET_UPDATES(DerivedFun, Value)

DerivedFun() {
  addInputDependency<BaseFun>(Update::Value, source, Output);
}
};

Here DerivedFun::Update::Value (=1) is different from BaseFun::Update::Value (=0), and only BaseFun::Update::Value has a registered update function. Yet it is possible to declare an input dependency for DerivedFun::Update::Value that does not have a corresponding update function. This will only fail at runtime if:

  • The update is called => crash in release, false assert in debug
  • The graph's Log is printed: there is no update corresponding to DerivedFun::Update::Value

Instead we should either decare

struct DerivedFun : public BaseFun
{
SET_UPDATES(DerivedFun, Value)

DerivedFun() {
  registerUpdates(Update::Value, &DerivedFun::updateValue_); // explicitely register the update for DerivedFun::Update::Value
  addInputDependency<BaseFun>(Update::Value, source, Output);
}
};

or

struct DerivedFun : public BaseFun
{
DerivedFun() {
  addInputDependency<BaseFun>(Update::Value, source, Output); // use update signal and function from parent class
}
};

This PR merely ensures that we cannot have a call to addInputDependency without a corresponding update function. It however forces the user to always call registerUpdates before addInputDependency, which in practice is already the case with only very few exceptions.

@arntanguy arntanguy force-pushed the topic/RegisterUpdates branch from a99a717 to 52d7959 Compare April 9, 2025 16:48
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.

1 participant