Generally this seems quite fine to me. Are you planning to address the remaining TODOs soon? I'd like to get this in for BCon2.

In D12423#335668, @Bastien Montagne (mont29) wrote:

Am not convinced there should be a new BKE item for that? It's extremely specific 'one case' usage, would have rather kept everything in wm_files_link.c tbh (except for the IDType callback). But no super-strong opinion on this either, if consensus is to get this new BKE 'module' I can live with it.

I think that if certain code has a very specific, single use case, it's great to have it in a separate BKE module. Putting that functionality between more generic code would be more confusing to me than clearly separating those.

Main strong bad thing in this patch for me is how update_file_path re-introduces ID-type specific behavior. The whole point of IDType and its callbacks is that all ID-specific code is kept there, and generic ID management code does not need to know actual ID type.

I agree, ID-type-specific code should be in the corresponding IDType's callbacks.

So think the new foreach_external_file callback should not use const file path, and add some extra info in case callback needs to edit the string, something along that line:
[...] Then callbacks can properly edit the string themselves when needed.

I don't see how this follows from the preceeding text. If there is no concrete goal to make things mutable, we shouldn't. Such mutability adds all kinds of complexities, and if those complexities aren't even used at the moment, it isn't tested either. Either callbacks won't take this flexibility into account, setting the stage for future bugs, or it'll take extra effort to take into account these use cases as well (and they still aren't testable, so there are guaranteed to be bugs there).

typedef void (*IDTypeForeachExternalFileFunctionCallback)(struct ID *id,
                                                          char **external_file_path,
                                                          size_t external_file_path_size,  /* In case it is an array of chars, 0 is dynamically allocated. */
                                                          const bool is_mem_alloc,  /* In case it is a dynamically allocated string (could also be a set of flags). */
                                                          void *user_data);

If we do keep the mutable paths, I would strongly recommend to not have a redundant boolean parameter. If external_file_path_size == 0 already signifies "dynamically allocated", we don't need to pass is_mem_alloc=true.

Having said that, I think we should only continue with this mutable external_file_path if we also clearly define how such mutations are supposed to be handled by the IDType::foreach_external_file functions. Until there is a clear design for this, with concrete & testable goals, I wouldn't make the API more complex than necessary.

TL;DR: Yes, this is extremely rushed in, last minute change that is anything but trivial... Like half of what was done last month for asset project. Not happy with it, already said it several times. Urgency type of work is never fun and never lead to best possible cleanest solutions, especially not if design is needed.

In other words, I would not mind at all to keep this for after 3.0.

In D12423#337956, @Sybren A. Stüvel (sybren) wrote:

[...]

So think the new foreach_external_file callback should not use const file path, and add some extra info in case callback needs to edit the string, something along that line:
[...] Then callbacks can properly edit the string themselves when needed.

I don't see how this follows from the preceeding text. If there is no concrete goal to make things mutable, we shouldn't. Such mutability adds all kinds of complexities, and if those complexities aren't even used at the moment, it isn't tested either. Either callbacks won't take this flexibility into account, setting the stage for future bugs, or it'll take extra effort to take into account these use cases as well (and they still aren't testable, so there are guaranteed to be bugs there).

I do not see how you want to allow modification of those paths otherwise? Which is the very purpose of this patch, and is a very common use-case of such iterator, see existing BKE_bpath_traverse_id() usages.
Just like BKE_library_foreach_ID_link() allows to modify the iterated ID pointers. Having immutable paths here is basically useless, since callback code will have to find itself again what to edit.
(As a side note, one of the later goals of proposed new foreach_external_file is to replace BKE_bpath_traverse_id too - but this is definitively not for 3.0)

If editing such path requires specific handling (beyond regular ID tagging in depsgraph), then the specific IDtype iterator code should handle it .

Which points out to another need for those IDTypeForeachExternalFileFunctionCallback callbacks: they should return whether they edited the string or not.

typedef void (*IDTypeForeachExternalFileFunctionCallback)(struct ID *id,
                                                          char **external_file_path,
                                                          size_t external_file_path_size,  /* In case it is an array of chars, 0 is dynamically allocated. */
                                                          const bool is_mem_alloc,  /* In case it is a dynamically allocated string (could also be a set of flags). */
                                                          void *user_data);

If we do keep the mutable paths, I would strongly recommend to not have a redundant boolean parameter. If external_file_path_size == 0 already signifies "dynamically allocated", we don't need to pass is_mem_alloc=true.

Having said that, I think we should only continue with this mutable external_file_path if we also clearly define how such mutations are supposed to be handled by the IDType::foreach_external_file functions. Until there is a clear design for this, with concrete & testable goals, I wouldn't make the API more complex than necessary.

Suggested the extra boolean for sakes of clarity (and even to have a proper set of enum flags, in case we'd need to pass more flags in the future)...

@Bastien Montagne (mont29) I do have an idea how to design an API to address the issues we described above. I will work on the design this week and see how we can move forward.
Note as this feature has moved to 3.1 we should also lower the priority.

I added a 'design' task for the API signature in T92315: API Design for `IDType::foreach_external_file`.