ofPolyline::updateCache() and const

Related to this post in an unrelated thread, if I wanted to avoid ofPolyline::updateCache() because the ofPolyline isn’t changing, I need to use const. Is the following the correct way to do this? My const-correctness skills are truly awful! So sorry and thanks for the help!

// a getter for a class
// the return and the function are both const if pline isn't changing
ofPolyline pline;
inline const glm::vec3 getPointAtIndex(float f) const {
return pline.getPointAtIndexInterpolated(f);
}

I think that looks correct. One way to check would be to add a print out to ofPolyline::updateCache() method just for testing, something like:

void ofPolyline_<T>::updateCache(bool bForceUpdate) const {
    if(bCacheIsDirty || bForceUpdate) {
		ofLogNotice() <<"Updating Cache: " << ofGetFrameNum();
 		...
1 Like

With some testing, I can’t seem to call the const version of ofPolyline::getVertices() with the following. Any ideas on how to call the const version?

ofPolyline pline; // in ofApp.h

const auto& vertices = pline.getVertices(); // prints "nonconst called"
const std::vector<glm::vec3>& moreVertices = pline.getVertices(); // prints "nonconst called"

and a modified ofPolyline.inl (for testing):

//----------------------------------------------------------
template<class T>
std::vector<T> & ofPolyline_<T>::getVertices(){
    flagHasChanged();
    ofLogNotice() << "nonconst called"; // added this line
	return points;
}

//----------------------------------------------------------
template<class T>
const std::vector<T> & ofPolyline_<T>::getVertices() const {
    ofLogNotice() << "const called"; // added this line
	return points;
}

I think the thing that you’re calling the function on has to be constant. That might not be practical for an actual ofPolyline object, but you could make a constant reference to it (or write a function that accepts a const ofPolyline& parameter), then call .getVertices() on that reference:

ofPolyline pline;
const ofPolyline& pline_ref= pline;
auto& verts= pline_ref.getVertices(); // should be the const version

I hope this is correct. I’m definitely learning here too. Also thanks for starting a new thread- I didn’t realize how off-topic I ended up being on the other one.

2 Likes

This is the correct answer. I had also assumed
const auto& vertices = poly.getVertices(); would work but you have to have a const ofPolyline reference to get the const functions.

1 Like

Hey thanks for the replies! And yes the following calls the const version of .getVertices():

ofPolyline pline; // ofApp.h

// make a const local reference to the pline
const ofPolyline& plineRef = pline;
// then get a const local reference of the vertices
const std::vector<glm::vec3>& vertices = plineRef.getVertices(); // prints "const called"

I think a reference should be fine, right? No copies are made; its local and goes out of scope; it can be passed as const to a function. Maybe this how it was intended to be used.

A lot of those wonderful functions in ofPolyline call updateCache(), and .getVertices().size is super useful too. The above pattern seems like the best way for const things to happen with the vertices in the polyline without also calling flagHasChanged(), which is eventually what can trigger updateCache().

terser C++17:

auto & vertices = std::as_const(pline).getVertices(); 

but… the original objective was “I want a const ref to the vertices, without flagging the cache”. why is that not possible without throwing the polyline in a const hoop?

the real question is why does the non-const getVertices() triggers the dirty cache? it presumes the vertices will be modified and not just accessed? should there be an explicit getNonMutableVertices() that returns & const and does not trigger dirty no matter the object constness?

because you can do

auto vertices = std::as_const(pline).getVertices();

and get a (mutable) copy of the vertices of a properly-const polyline… so it’s kind of mixing 2 problems.

(edit because accidentally published before done)

and actually the documentation of getVertices() says

Gets a vector of vertices that the line contains

which sounds more like either a copy or an immutable list than a mutable access to the underlying points…

is there a true need to provide a mutable access to the points of ofPolyLine? (i.e. what the non-const currently does)

Hey this is a nice and clear way to do it! I’ve not seen std::as_const() before; it does need c++17 so it will work with the nightly build. It does indeed call the const version of .getVertices().

It seems like a helpful and common design pattern in OF is to be able to do stuff like this:

ofPolyline pline;
std::vector<glm::vec3> nextVertices(100); // make some new vertices in a vector
pline.getVertices() = nextVertices; // put them into pline by assigning them to a reference

So yes I think it assumes that the nonconst reference returned by .getVertices() could modify the vector. The class is set up to recalculate some stuff if the vertices have potentially changed and the need arises (with some of its other functions).

An alternate function could work great, but then just explicitly getting a const reference seems OK and clear (at least to me) now that I know how to use it. Like maybe some others, I thought incorrectly that const auto& vertices = pline.getVertices() would give a const reference to the vector, like it does in a range-based for loop

I think this makes a copy. References are awesome for the larger classes and containers. While maybe lacking in const-rigor, references eliminate the cost of making copies and can really speed things up.

Yes totally! Like the quick example above, sometimes you want to just take a whole new vector of different values and plop them into an ofPolyline. The same is true with vectors in ofMesh and ofPixels. Maybe the vertices are coming from an ofFbo that has been updated with a shader, or the ofPixels from an image or video, or maybe even from outside OF via OSC or an Arduino. The alternative is to loop thru and copy each vertex, one at a time, which can be painfully slow with large vectors if it happens often.

Another big reason is multithreading with c-style arrays. A vector can be copied to an array, modified by a thread, and (basically) copied back into an ofPolyline in a big chunk using the non-const reference. I just did a study on this that was inspired by @dimitre. It seems to work great! But the reference was the fastest way to get the values back into an ofPolyline (relative to a loop or std::move()).

to be clear: yes the (non-&) “auto vertices” takes a copy of the returned const & vector.

I see the need for setting in bulk points generated elsewhere (vs random assigning them under the rug as v[3]={.1,.2,.3}; ) and the non-const getVertices() allows that – and it then makes sens to trigger the cache? (I was thinking of a situation where you want to read-only iterate the values, for instance exporting, or building something else with the data – then you want it without cache trigger).

I still consider it strange that merely getting a reference to data member triggers the cache. it is effectively a hidden side-effect for an hypothetical future assignment.

I would then see it better as if getVertices() always returns “const &” without cache side-effects (and without resorting to hacking the constness of the polyline instance which is unrelated to the problem), and have a version of setVertices(&my_fresh_vector) that assigns your fresh vector (as efficient as the compiler will make it – and better than clear() and insert()), and triggers the cache stuff.

These are some good ideas, and they might make ofPolyline a bit more consistent with ofMesh, which has various " .addData()" methods that take an array or std::vector.

Another idea might be to add a public interface for ofPolyline::bCacheIsDirty, so that its accessible with a setter or thru relevant functions. But, you’d still have to know what it is and how to use it within the class. I think this is how I solved the issue I had previously with updadCache() running when it didn’t need to run.

But making a const reference is super easy, clear, and only needs to be done in the case where you don’t want updateCache() to run for some reason.

If you like you could start a pull request and maybe get a discussion started for some longer-term changes to this class. I’m not sure but I think this how development stuff gets hashed out within the community.