I propose to add refineDetectedMarkersMultiDict overload with dictionaries id like in detect. May be useful for multi-scale markers.

Made sure refine is now looping over the markers and checked the output. Looks like this can be done safely without having to have two functions.

1. In my opinion, methods get(set)Dictionary, add(remove)Dictionary are redundant. It is enough to have get(set)Dictionaries. Perhaps it would be better to preserve old get(set)Dictionary, so that they would return the first one.

2. Is it safe to return reference to internal vector in method getDictionaries? Maybe it would be better to return a copy.

3. What about corner cases: empty vector, duplicates?

4. Methods should be documented.

Hey, thanks for the feedback.

1. yeah, add/remove may be a bit redundant. I went for maximal convenience here so the dictionaries list can be manipulated in many ways. I want to point out though that the default behavior of get/setDictionary (so without the optional index) is preserved.
2. As long as it's const it's safe. You can always get a copy, rather than a view of the insides by doing vector<Dictionary> copy = detector.getDictionaries() rather than vector<Dictionary>& reference = detector.getDictionary(). So in order to not do a copy you have to specify that you're taking a reference. If you're asking for safety in terms of a malevolent attacker rather than a potential source of a bug, then yes, returning a copy would be better.
3. Empty vector isn't allowed. I have a check in the constructor and the removeDictionary() function for that, but I forgot to check in the setDictionaries(). Thanks for spotting this! On duplicates I purposefully did not add a check and leave some of the responsibilities to the user. I could change this to use an unsorted_set internally that automatically ignores duplicates, but the interface would remain a vector. This would also mean that getDictionaries() would return a copy for sure. Even then you could do nonsensical stuff like running DICT_4X4_50 and then DICT_4X4_100 and potentially find a lot of duplicated markers. This can of course be solved by just running the larger dictionary and ignoring the smaller one, but that's a lot of hand-holding that I am not used to with the OpenCV API.
4. Sure, makes sense. I added some non-trivial complexity after all.

So for now I will fix the bug that allows for setting an empty vector and I'll add documentation. On the rest I'd like some confirmation for my suggestions and then I'm happy to make the changes.

1. Old get/setDictionary API would work, yes. But ABI compatibility would be broken. Can we avoid it?
2. No I meant safety in terms of exposing internal structure. For example it allows user to create iterator which might become invalid suddenly. Also we won't be able to change internal data structure.
3. Each access function must be tested for possible errors then.

1. Alright, I've updated the PR to keep the behavior of the set/get functions and the ABI the same.
2. Yeah, good point with the iterators. I make it return a copy now.
3. I think I had tests already, but since you asked about it earlier, the most current version now has no add/remove functions and thus it's a lot easier to use and also easier to test.

I hope this addresses all the concerns. I left a bit of documentation for the set/getDictionary functions so it's clear what they do with the dictionaries list.

Wouldn't it be more convenient to modify Dictionary class, so that it would be able to combine several dictionaries underneath. It could also handle duplicate markers and provide convenient indexing. Detector interface would stay the same and users would require minimal changes in their code.

Not a bad idea, but the Dictionary class is also used for these calibration boards and the Charuco stuff as well. Its public members (errorCorrection, markerSize, bytesList) would all need to change because they would be ambiguous. Just from a quick check I think such a change would be more disruptive than the interface change suggested in this PR.

I think such a change would be more disruptive than the interface change suggested in this PR.

Agree and support

We already have function extendDictionary which adds randomized markers to the existing dictionary, so I believe creating similar function to combine several dictionaries with the same marker size would be quite easy. Or do we need to have dictionaries with different marker sizes?

At least for the use case we have in mind you would want to scan dictionaries of different marker sizes, yes.

I believe (de)serialization is not tested. Is old format supported? I.e. if we have just one dictionary, would older versions of OpenCV be able to read it?

True, I couldn't find a unit test for it, but I tested it locally. I can clean the code up and add it as a test.

I made sure it could read the old format, but it wouldn't produce the old format if there's only one dictionary. I'll add that, so both read and write support the old format if there is only on dict.

Why not regular std::set?

made it a map now as marker size needs to map to candidate tree. I made this connection now more deliberately and direct so I also don't need to calculate indices for other containers which may cause bounds issues as you correctly pointed out.

Please use cv::tempfile for file name and make sure file is removed using remove at the end (no ASSERT_ can be used).

Alternatively you can try to use in-memory storage:

Thanks, that's what I was looking for.

Failure to open file should become test failure. Here and in other places.

Perhaps it would be useful to define and test behavior in case when several dictionaries contain same markers.

Good idea. Added a test that shows off how you can detect three markers for 2 similar dictionaries.

If I understand correctly, this function filters candidates using marker size as a hint to estimate distances or something. Could dictionaries with different marker sizes affect detections of each other?

That was also my worry and when reading into the code of the function it may affect candidates in some corner cases. This is why I'm making copies of the vectors before filtering, so the order of identifying with multiple dictionaries doesn't matter. The copies are an overhead though that I'd like to avoid.

Can dictionary have marker size other than 4, 5, 6, 7? I don't see any checks in Dictionary constructor or other places. If it can, then we have possible out-of-bounds access.

Yeah, I was thinking about the predefined dictionaries only, but custom dictionaries could potentially break this. I made it more flexible that now any size of marker is supported.