BaseContainer::GetCustomDataTypeWritableObsolete()... any side effects?

fwilleke80

Hello,

I'm trying to track down a random crash in my code, that only occurs in C4D 2024. While I'm manipulating a CustomGui in my ObjectData's Description (e.g. by dragging a slider in that CustomGui), the ObjectData's GetVirtualObjects() is called, of course. In that function, I get the CustomGUI's data. Sometimes, that data is thrashed. Only happens during ongoing GUI interaction.
While investigating, I found this API code in c4d_basecontainer.h:

template <typename DATATYPE> DATATYPE* GetCustomDataTypeWritableObsolete(Int32 id)
{
	// TODO: (Tilo) needs to be rewritten for COW support because this modifies data inside the container data
	const GeData& dat = GetData(id);
	return MAXON_REMOVE_CONST(dat).GetCustomDataTypeWritable<DATATYPE>();
}

This seems to be the only way to get a writable pointer to a CustomDataType in a BaseContainer. Is it? Because, that comment sounds portentous.

Thanks in advance!

Cheers,
Frank

fwilleke80

Any news on this?

m_adam

Hi sorry I was a very packed last days.

Regarding your issue, this is correct this is the only way to get a Writable pointer to a CustomDataType in a BaseContainer. I know in some case it is not as easy as it is said, but GetVirtualObject should really just "read" parameter and output a geometry. Modifying the basecontainer is something that should be avoided since GetVirtualObject is called from different thread and therefor it is not too surprising that it crash.

The correct way would be to override the Execute method and do your change or even do your change from the main thread with ExecuteOnMainThread.

Cheers,
Maxime.

fwilleke80

I am not modifying any data from within GetVirtualObjects(), of course not.

But Tilo's comment in the code sounds like the data is modified anyway, just by calling GetCustomDataTypeWritableObsolete(). "needs to be rewritten for COW support because this modifies data inside the container data" sounds pretty clear, that's why I'm asking.

Cheers,
Frank

ferdinand

Hey @fwilleke80,

I am still on vacation (and the thread is therefore still owned by Maxime), I just thought I would point out some stuff:

When I documented the 2024 API changes I also stumbled upon the name of this function. I then asked Tilo and he then told me what now can be found in his TODO comment. The function is obsolete because it should treat custom data as COW but it does not, it uses one shared instance.
The function is more or less perfectly fine, it 'only' violates the const-correctness of its underlying data by not treating them as immutable all the time. There is no 'data modified anyway' this function just removes the constness of the internal data so that a user could modify it. This would be better solved with COW, hence the name and comment.

I am also not quite sure about the exact nature of your problem. You use both the words crash and 'trash' (was that a typo?) but I assume you are crashing with an access violation on possibly deleted data. And you seem to crash from the main-thread? Or are you off main-thread? That is all a bit cloudy. You should post your concrete code and stack trace, otherwise it is as always hard to help you.

When you only want to read data, you must use the read only variant BaseContrainer::GetCustomDataType of the method you are using. Fixing this will very likely not fix your crash but sticking to const-correctness is important nonetheless.
When you are still running into access violations from a non-main thread method, you could try to overwrite NodeData::GetAccessedObjects just to see if it makes any difference if you manually claim a read-dependency relation to that parameter. But that should not be necessary to be done because Cinema should mark everything as a dependency when you do not implement the method manually. If overwriting the method would fix your problem, you would have found a bug which would be critical to be reported and fixed. See Node Data Access for details on the subject.

While I would not categorically rule out that there is a bug in custom data type access in NodeData instances, it seems quite unlikely that this is the case, given how ubiquitous custom data types are in our API. You should share your code because otherwise we cannot help you.

Cheers,
Ferdinand

fwilleke80

Hi Ferdinand,
happy new year! Thanks for chiming in! I have fixed the crash in the meantime (don't ask how, it was all quite confusing, but it might have had to do with how I got the BaseContainer in the first place.

So, if I understand this correctly, most of the structures in C4D 2024 that use COW will create a new instance when getting a writable reference or pointer, while getting a const ref or pointer will use a shared instance? And BaseContainer::GetCustomDataTypeWritableObsolete() will not do that, but still allow me to modify the pointed data? I can live with that

Posting code is difficult, as everything is part of a pretty large project. I'm sorry my questions are sometimes a bit shadowy.

Cheers,
Frank

m_adam

@fwilleke80 said in BaseContainer::GetCustomDataTypeWritableObsolete()... any side effects?:

So, if I understand this correctly, most of the structures in C4D 2024 that use COW will create a new instance when getting a writable reference or pointer, while getting a const ref or pointer will use a shared instance? And BaseContainer::GetCustomDataTypeWritableObsolete() will not do that, but still allow me to modify the pointed data? I can live with that

Exactly !

Cheers,
Maxime.

ferdinand

Hey @fwilleke80,

a happy new year to you too, I was even in Berlin at New Years eve

Regarding COW, I would recommend having a look at this code example in our docs, as I tried to untangle the exact subject there.

When you are accessing raw data such as an array of Vector* where there are two methods: Vector* GetVectors() and const Vector* GetVectors () const, you can be fairly sure that calling Vector* GetVectors() will trigger a copy on write event. Cinema 4D has no means to detect what you are doing with this writeable array; if you only read it or if you also write it. Therefore, all data is copied as soon as you attempt to access the writeable data. We should be very careful with requesting write access in such cases because it can be a VERY expensive operation.
This is opposed by (custom) data types such as BaseSelect. There is a const BaseSelect* PolygonObject::GetPolygonS const and a BaseSelect* PolygonObject::GetWritablePolygonS. But since the data is wrapped here by this special data type, requesting access to the writeable BaseSelect alone will not trigger COW. It is only when you then call one of the non-const methods of this instance that its underlying data is copied.

So, in general, (custom) data types are an indicator that accessing the writeable data is not wildly expensive (because the copying can be postponed to the call of a non-const method), while raw data such as arrays (e.g., all point data) will be copied as soon as one requests write access. There are some exceptions to this, such as that copying will only occur when there is another reference holder/user to that data.

But when we are dealing with abstract/templated custom data types such as with GetCustomDataTypeWritableObsolete, we do not know in advance how the read/write behavior of the returned type is. So, one would have to draw a copy before returning a non-const pointer to be sure. And, yes, :: GetCustomDataTypeWritableObsolete is currently not doing that. It simply removes the constness of the underlying data which can lead to crashes when someone or something is trying to treat such data as if it would be thread-safe/COW.

Cheers,
Ferdinand

fwilleke80

Hi Ferdinand, thank you for the clarification!