Regression: ofParameter<ParameterType> fails to compile if ParameterType lacks operator== due to isInit()

[eduardfrigola]

Hi!

While testing for the 0.12.1 release, I found a regression in ofParameter related to the newly introduced ::isInit() method in commit e2bb65c

Currently, for this method to compile, ParameterType must implement operator==

openFrameworks/libs/openFrameworks/types/ofParameter.h

Lines 896 to 898 in c9561a0

	bool ofParameter<ParameterType>::isInit() const {
	return obj->value == obj->init;
	}

Minimal example to reproduce issue.

//Does not compile
struct foo{
    int x;
};
ofParameter<foo> pFoo;

//Does compile
struct bar{
    int x;
    bool operator==(const bar& other) const{return x == other.x;}
};
ofParameter<bar> pBar;

In 0.12.0, both foo and bar compiled without issues.

I realize my use case for ofParameter goes beyond its intended design (I'm using it in ofxOceanode for sharing data between nodes). Some past issues I've encountered with ofParameter (such as #6576) have been related to this extended usage.

If you think this regression should be addressed, one potential approach could be similar to how ::fromString / ::toString handle types that lack stream operators.

Relevant parts of the ::fromString implementation:

openFrameworks/libs/openFrameworks/types/ofParameter.h

Lines 930 to 937 in c9561a0

	template <typename ParameterType>
	inline void ofParameter<ParameterType>::fromString(const std::string & str) {
	try {
	set(of::priv::fromStringImpl<ParameterType>(str));
	} catch (...) {
	ofLogError("ofParameter") << "Trying to de-serialize non-serializable parameter";
	}
	}

openFrameworks/libs/openFrameworks/types/ofParameter.h

Lines 479 to 482 in c9561a0

	template <typename ParameterType>
	typename std::enable_if<of::priv::has_loading_support<ParameterType>::value, ParameterType>::type fromStringImpl(const std::string & str) {
	return ofFromString<ParameterType>(str);
	}

openFrameworks/libs/openFrameworks/types/ofParameter.h

Lines 448 to 453 in c9561a0

	template <typename T>
	struct has_loading_support {
	static std::istream & stream;
	static T & x;
	static constexpr bool value = sizeof(check_op(stream >> x)) == sizeof(yes);
	};

cc @alexandreburton @artificiel What do you think? Would you like me to submit a PR to implement this approach?

[artificiel]

good case — a type trait would work:

template <typename T>
struct is_comparable<T, std::void_t<decltype(std::declval<T&>() == std::declval<T&>())>>
	: std::true_type {};

but there is a difference vs has_loading_support that isInit() is currently a pure virtual of ofParameter, so we can't disable it in ofParameter (it becomes incomplete). but in the abstract, it may be switched to not-pure with a default of returning true (which is what it does on the exception for ofParameterReadOnly, for instance). this means calling isInit() on the abstract of a foo would return true (eg ofParameterGroup enumeration), and pFoo.isInit() would fail to compile.

I don't see what would be affected by the non-purity of isInit(), as long as it's virtual and can be called on all? (I would also disable the init copy so objects that are not comparable are not copied on construction).

note that #7900 has been in-flight for more than a year, addressing tweaks around this topic. @ofTheo possible to merge #7900 so the solution here can build upon it?

[ofTheo]

@artificiel happy to merge #7900 if there aren't expected to be any regressions.
Also happy if you want to coordinate with @eduardfrigola on a PR for this issue.

Thanks!!

[artificiel]

@ofTheo #7900 is about "touching up" the (new) isInit()/reInit() stuff so it's within what happened between 12.0 and 12.1.

this issue is a true regression and while fixing it is not difficult in itself, it's preferable to start from #7900. with @eduardfrigola testing it against ofxOceanode it should cover both casual and complex usage.

[ofTheo]

Thanks @artificiel!
Merged.

[artificiel]

@eduardfrigola https://github.com/artificiel/openFrameworks/tree/ofParameter-moreinit passes the CI tests and compiles some basic cases I tried including your struct as top post.

all in all it's better as it makes ofReadOnlyParameter and <void> not exceptions anymore.

also note that you may do:

ofParameter<float>::init_opt_out = true;

which will opt-out floats from being copied for init comparisons. might be good for large objects you don't want to reinit.

prior to OF PR i will provide ofxTests and maybe revise a bit the layout of things and compile more projects (and I will take a look how it interacts with ofEvents) but if you can confirm the strategy works for your use cases it would be great.

[eduardfrigola]

Hi all!

Thanks for moving this forward! I'll run further tests with my setup (I need to manually merge our branch) to ensure everything works as expected.
Looking at your proposal and the discussion around making ::isInit() a non-pure function, I find that my use case leans in the opposite direction.

in ofxOceanode we have ofxOceanodeParameter<T> and ofxOceanodeAbstractParameter ,which derives from ofAbstractParameter. There are impelemented here. With this aproach (which is not recommended by arturo) has worked well in practice. One risk with this setup is that I could easily overlook re-implementing ::isInit() in ofxOceanodeParameter.

While I agree it's not the cleanest approach (and has an ongoing refactor), the fact that new pure virtual methods in ofAbstractParameter trigger compile-time errors has been really helpful in catching missing implementations.

That said, your solution fixes the example I provided, and I believe it resolves the issue. However, I’m not fully convinced why isInit() needs to be non-pure in your implementation. Since you're not deactivating the function, it remains defined for all types. I tested making isInit() pure virtual again, and everything seemed to work fine.

Could you clarify the reasoning behind this change? Are there cases where keeping isInit() pure would cause issues?

Thanks again for your work on this!

[artificiel]

@eduardfrigola it's true that as implemented most things will work if the functions are kept pure (including reInit() = 0 which was one of the objectives of #7900), and it's because I ended up working with if constexpr instead of SFINAE. this leaves more liberty (including run-time opt-out with ::init_opt_out), with the downside of having no-op calls (vs completely absent methods that prevent compilation).

in effect it provides a low-cost decent default behaviour for what is probably exceptional usage. there is also a gain with what were exceptions for ofReadOnlyParameter<> and ofParameter<void> (which are important internally to OF) now become generalized by this new "norm".

ofParameter in itself is a unique pattern in OF, and ofxOceanode subclassing it even more so! the important aspect is to support your use case. the pure virtuality of isInit()/reinit() was introduced by myself, so I really don't feel bad about backtracking after being aware of the consequence on your deployed cases.

but let's give it some time on the back-burner — knowing the branch works for you, I'll throw more things and thoughts at it, and propose an OF PR in the coming days prior to 12.1.

[artificiel]

OK so having non-pure isInit()/reInit() has another good side-effect is that is allows non-copyable ofParameterType (for instance, ofTexture).

so with pure version, compile errors forcing implementation on derived classes (as you) is one thing, but actually requiring all ofParameterTypes to be copyable is another... and was an overlook in the design of the init feature (which is really along the idea of a parameter space as usually conceived in an ofxPanel UX context). however if we want to have recursive isInit/reInit with ofParameterGroup it means it has to be implemented at the Abstract level... (because Group operates at the abstract level).

it might be not the greatest thing in the world to have no-op functions, but with logging the uses it means people expecting isInit() to work eg: on ofTexture will be notified it's not doing anything (just like ofParameter<void> already does, as a special exception case).

also having the possibility to opt-out by signalling ofParameter<MyHeavyAndCopyableClass>::init_opt_out = true; on the static level is a decent feature.

thinking along this, maybe there is a better possible design, perhaps a isInitable trait that encompasses comparison, copy, and maybe even a compile-time opt-out flag for custom classes that are copyable yet don't want to be subject to the isInit() copy, but with 12.1 approaching I would go with https://github.com/artificiel/openFrameworks/tree/ofParameter-moreinit now as #8325 so uses cases such as ofxOceanode are functional in 12.1.

[eduardfrigola]

Hi @artificiel ,
Good catch on non-copyable types problem.

I'm trying to use ofxOceanode with your pull to test my use case, manually adding the pulls #6614 and #7122 needed for ofxOceanode to work.

I'm finding more issues that I'm reporting here. But as more issues apear, more certain I am that my use of ofParameter should not be prioritised, and we should try to stabilise it's functionality with ofxGui and ofxImGui as it was it's design idea.

I know I need to change my engine logic away from ofParameter, and so, not having support for ParameterTypes that are not copyable seems that a reasonable thing for a class that it's design idea was to be used with a gui system for manipulation (AFAIK).

Thanks again for the effort on this and improving ofParameter for everyone.
Eduard

[eduardfrigola]

Closed by #8325