Now that f799a29 fixes the demo hash collision I see no reason not to use FNV1a hash algorithm and incompatibility commented in point 7 of my original post

So I'm force-pushing a version of this PR with this FNV1a instead of FastHash64, the code is simpler, faster and properly checks for the ### mark. My home-made measurements give about 25%-50% faster than CRC32. Hey, if it proves worthy, we could even implement the hash32 version by doing the FNV1a and folding the 64->32 bits: best of both worlds.

Anybody feel free to comment on either version of the PR, of course.

Thank you, I'll try to review next week.
Some quick thought:

• See if the string parser in ShowDebugLogWindow() still works.
• May want to test this with the Test Suite.
• May need to do a cursory check about sscanf() working on all compilers/libs.
• Unsure if %016llX works everywhere too. I don't think 64-bit integers in are well standardized. Notice this blocks in GDataTypeInfos[]:

#ifdef _MSC_VER
    { sizeof(ImS64),            "S64",  "%I64d","%I64d" },  // ImGuiDataType_S64
    { sizeof(ImU64),            "U64",  "%I64u","%I64u" },
#else
    { sizeof(ImS64),            "S64",  "%lld", "%lld"  },  // ImGuiDataType_S64
    { sizeof(ImU64),            "U64",  "%llu", "%llu"  },
#endif

BUT would need to check history/blame about this. As we raised our build requirement to a subset of C++11 (VS2012 ?) in 1.87 it might be easier now. I'll be able to check this later if you don't have millions of compilers installed (I do).

• If this work I think we can get simply rid of CRC32.

Obviously the slight yet biggest annoyance are the IMGUI_HASH_FMT things...

Don't act on this idea yet, but I am wondering if it would make sense adding a helper relying on constructor/destructor to optimally fill a static buffer (with a small number of slots) and return a const char*, so it becomes "%s", ...., ImFormatID(id). and destructor release its slot. It would need to be a function with static return type, as a class constructor won't auto-cast to const char* in an ellipsis function.

May want to test this with the Test Suite.

There is a test suite?! I see it now, I'll try and play with it.

I'll be able to check this later if you don't have millions of compilers installed.

I don't have so many, I just tested g++ and clang++, they seem to work fine.

Personally I would prefer to define ImU32 and ImU64 in terms of uint32_t and uint64_t, from <inttypes.h> that exists in C++98, IIRC. Then, instead of "llX" we can use the proper PRIX64 that is guaranteed to exist whenever there is support for 64-bits and do the right thing. That is:

#include <inttypes.h>
#ifdef IMGUI_USE_HASH64
typedef uint64_t ImGuiID;
#define IMGUI_HASH_FMT "%016" PRIX64
#else
typedef uint32_t ImGuiID;
#define IMGUI_HASH_FMT "%08" PRIX32
#endif
// ditto for ImS8, ImU8, ImS16, ImU16, ...

If this work I think we can get simply rid of CRC32.

That would make things simpler. I did the option because I thought maybe there are embedded or old systems out there without proper support for 64 bit integers.

I am wondering if it would make sense adding a helper relying on ... static buffers ...

Why a static buffer? You can use an automatic one and avoid the destructor altogether. Something like this:

struct ImFormatID_
{
    char txt[17];
    ImFormatID_(ImGuiID id)
    {
        sprintf(txt, "%016X", id);
    }
};
#define ImFormatID(id) ImFormatID_(id).txt

This is risky because that macro returns a pointer to a temporary, but that is the beauty of C++, I guess:

printf("%s\n", ImFormatID(0x1234)); //ok
const char *txt = ImFormatID(0x1234); //dangling pointer!

Didn’t think of a macro wrapper, that’s better for the risk of dangling pointer is too high with this technique.

I think PRIX64 will work just fine on any 64-bit type.
My wonder is about whether its content converged accross compilers since C++11 of if there is a single format type that willl work accross compilers for a 64-bits integer. I’ll try to investigate next week.

Minor: as part of e816bc6 I merged the signature fix for ImHashXXX functions using ImGuIID seed and the use of 0u in that popup function. May trivially conflicts on your end.

I just saw this PR and thought i would share my (very similar) patch against the docking branch.
I found 2 local buffers that have to be increased in size for the longer hash values.
The path should apply cleanly to the current docking commit ea45653.
imgui_fnv.patch

crc has the advantage of having intrinsic implementations available..(It would be nice if the ImGuiID were 64bit though.)
I have replaced imgui's original CRC with this:

crc_example.txt

^ You would think github of all places could properly handle displaying code, it has a add code button but apparently it does not work properly. Gave up and added it as a text file

^ You would think github of all places could properly handle displaying code

The code button does inline code by default unless you have multiple lines selected.

```cpp
printf("Multi-line codeblocks use three backticks (with an optional language specifier on the first line.\n");
```

Here's your CRC example properly formatted:

//      32/64 bit test msvc and intel       32/64 bit test clang,gcc,intel(linux)                    
#if (defined(_M_IX86)||defined(_M_X64) || defined(__i386__)||defined(__x86_64__)) && (defined(__SSE4_2__) || defined(__AVX__) ) //msvc doesn't define __SSE4_2__, so use next available __AVX__, which implies sse4.2 support
ImGuiID ImHashData(const void* data_p, size_t data_size, ImU32 seed)
{
    unsigned char* data = (unsigned char*)data_p, * data_end = (unsigned char*)data_p + data_size;

    ImU32 crc = ~seed;

#if defined(_M_X64) || defined(__x86_64__) // 32bit mode doesn't have 64bit crc32
    uint64_t crc64 = crc;
    for (; (data + 8) <= data_end; data += 8)
        crc64 = _mm_crc32_u64(crc64, *(uint64_t*)data);
    crc = (ImU32)crc64;
#endif
    for (; (data + 4) <= data_end; data += 4)
        crc = _mm_crc32_u32(crc, *(ImU32*)data);

    for (; data != data_end; ++data)
        crc = _mm_crc32_u8(crc, *data);

    return ~crc;
}
#else 
//original imgui impl  here

#endif

I wonder if it might be easier to just add an advanced config option that leaves ImHashData/ImHashStr undefined so that a user-provided implementation can be used instead.

FNV1a would be an improvement over CRC32, but it's hardly state of the art anymore. The 32-bit variant of xxHash is supposedly a much higher quality hash with significantly better performance while still being 32-bits.

xxHash is probably too big to distribute alongside Dear ImGui (although the main library is single header), but an add-on like imgui_freetype might be more appropriate.

That CPU-assisted CRC32 change seems surprisingly viable to consider as a temporary step.

xxHash is probably too big to distribute alongside Dear ImGui

While not aiming to hard-preventing it, I'd be in favor of not encouraging people to swap that hashing function, nor adding too many compile-time features that will make their way into package managers, bindings, and complexify things etc. If we settle on a nicer hash function, we'd be good enough. Intuitively I imagine that nicer hash may be 64-bits but I haven't deeply investigated trade-offs. Also note our use case are: very small data, very frequent calls (so code size/prologue/epilogue may have meaningful effect), and things may be not benchmarked enough for that favor.

That CPU-assisted CRC32 change seems surprisingly viable to consider as a temporary step.

That intrinsic looks great, but note that for ImHashStr you need to check for the ### so we have to either do the strstr() call beforehand or advance one byte at a time with _mm_crc32_u8().

Also, AFAIK _mm_crc32_u* are only available on SSE4.1 or later. Not exactly new, but probably too risky for the default code, unless we include a run-time check and a software.