Can we avoid this duplication? Maybe a fallthrough (first the SVM_TILES case determines the ID, then falls through to SVM which does the lookup and this part)? Or maybe merge SVM and SVM_TILES together and just check svm_slots[0].z == -1 for the un-tiled case?

This code seems to fall back to the first tile if the correct one is missing, is that intentional?

We probably still want to initialize svm_slots to some form of invalid value (e.g. make_int4(-1, -1, -1, -1)) since the code below accesses svm_slots[0] without boundary checking.

Also, I think we want to keep the option of passing in a single slot for simple cases, so maybe one constructor that takes an int svm_slot = 1 and initializes svm_slots from that, and one that directly takes a vector<int4>?

Nitpick: I'd prefer if this just returns the slots instead of filling the reference.

Hm, I'm not really a fan of creating handle outside and then repeatedly calling add_image. Maybe we could have an overload of add_image that accepts a vector<ImageLoader*> instead?

Since the type is only determined by the tile count, it probably makes more sense to just check slots[0].z instead of adding another type.

Combining the comments above, this can be simplified.

Maybe rename to parameter_texture_svm to make it clearer what the difference is?

Above, in the !ibuf case, ok gets set to false but the code now continues instead of exiting as before. I assume that's intentional. However, if ok == false, the RHS of && is skipped, so we never call image_memorypack_imbuf anyways. Probably just swap the order?

Why this change?

I think we also want to copy those in copy_image_packedfiles?

Based on the code inside svm_image_texture, I've added a similar check against -1 here in order to return the missing texture color.

The 'view' and 'tile_number' fields are on ImagePackedFile, not PackedFile so setting them in that method isn't possible. I double-checked that these fields are set everywhere ImagePackedFile is created though.

Good question, but I can only infer the true intent of this function myself. It _seems_ like this function prefers to Save to Disk if possible rather than doing a memory pack for dirty images. It has a check against IMA_SRC_GENERATED which already made it different than other code, so I made an equivalent check for tiled textures which is what's there now.

The scenario that triggers this code path is: Create an image (tiled or normal), paint on it, Save the .blend, Quit --> Select "Save" for the modified images. If the images had a filepath, they would be saved to disk at this step. If not, they will be memory packed.

Ah, indeed. Sorry, misread the code :(

Thanks for clarifying the scenario.

However, I am still confused why tiled images are handled differently here from "normal" images? Is it because it is not possible to have "normal" image without a file path, but it is possible to have tiled image without a file path? If the latter is correct it would save a lot of brain cycles if we put the information into a comment around the check.

Right, a "normal" image will transition from IMA_SRC_GENERATED to IMA_SRC_FILE once it's saved by the user (or was loaded from disk to begin with) which implies it has a file path at that point. Tiled images are immediately IMA_SRC_TILED upon creation so we can't use the source alone to determine if we should pack in this case or not.

Perhaps this function needs a better name. It's not strictly about determining if packing to memory is possible. It's more like bool image_can_pack_during_save_all(...) as that's all it's used for. Would that rename help (in addition to a comment maybe)?

Would that rename help (in addition to a comment maybe)?

Absolutely! :)