Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collapsing header doesn't push ID ! #256

Closed
ebriney opened this issue Jul 1, 2015 · 5 comments
Closed

Collapsing header doesn't push ID ! #256

ebriney opened this issue Jul 1, 2015 · 5 comments

Comments

@ebriney
Copy link

ebriney commented Jul 1, 2015

Hi Omar,
I did some some few tests with the latest version of imgui.

I fix almost every annoying ID issues with the new void* handle hashing which is great to desambiguate most situations relative to trees.

In some of my UIs i use Collapsing headers and must manually add Push/Pop ID if i have shared elements under collapsing. And there is no overload with the void* ID.

Thanks for your great job as usual.

@ocornut
Copy link
Owner

ocornut commented Jul 1, 2015

Yes that's because there's no "pop" for the collapsing header so it doesn't modify the state. Unfortunately I don't think it can / should be changed at this point.

@ebriney
Copy link
Author

ebriney commented Jul 1, 2015

Ok, thanks, I understand, you have no way to figure out the pop with current interface.

Concerning hash function, I can share you the implementation I used after trying a lot of things.

For string hashing, I used the one found in Emil Persson (Head of Research at Avalanche Studios) framework : http://www.humus.name/index.php?page=Comments&ID=296&start=0
very efficient, i never got any problem with it.

I replace the hash function with a more optimized one because on some processors multiply can have a latency of few cycles, stalling the CPU :

inline uint64_t HashMe64(uint64_t h, uint64_t c)
{
return ((h << 5) + h) + c; //h*33+c (djb2 hash function)
}

inline uint32_t HashMe32(uint32_t h, uint32_t c)
{
return ((h << 5) + h) + c; //h*33+c (djb2 hash function)
}

inline size_t HashMePtr(size_t h, size_t c)
{
return ((h << 5) + h) + c; //h*33+c (djb2 hash function)
}

@ocornut
Copy link
Owner

ocornut commented Jul 1, 2015

Did you have performance problems with CRC32?
Will look into replacing CRC32 but last time I tried to setup a test the total cost of all CRC32 was barely measurable.

From reading your message it is suggesting that you are hashing things yourself to form a void* ? What does your code looks like?
void* is really for passing a pointer to be hashed. If you really to build a stack to create hashes you can use PushID()/PopID().

(My intuition is that replacing *33 by *32 would pretty much defeat a lot of the hashing properties)

Was considering FNV1a which is what a few knowledgeable people recommended to me:
http://www.isthe.com/chongo/tech/comp/fnv/

@ocornut
Copy link
Owner

ocornut commented Jul 2, 2015

Closing this as the hashing discussion doesn't seem to relate to the stated issue (but we can continue discussing it).

@ocornut ocornut closed this as completed Jul 2, 2015
@ebriney
Copy link
Author

ebriney commented Jul 3, 2015

I had no performance issues with CRC32 and no collision with a lot of nodes (~1000) so it's fine to keep it (3d buffers generation should take a lot more time :) ).

I didn't understand what do you mean with void_.
In imgui I use the new prototype you add with void_ as ID and use the address of my objects in treeviews.

FNV1a hash function looks pretty good too (very similar than djb2).
dbj2 variant use xor instead of add too but that does the same, everything is in the 33 constant,
sdbm use 65599 instead for similar results.

idbrii added a commit to idbrii/cpp-imgui that referenced this issue Mar 24, 2025
Includes my merged PRs and everything in my dev branch. Haven't tested
with it yet.

Changelog:
Test case for clip rect
HACK: more recent Windows SDK and VS2017; disable graph
Set size to amount of space required
Merge pull request ocornut#349 from maksw2/master
Merge pull request ocornut#347 from mgerhardy/341
Merge pull request ocornut#348 from mgerhardy/fixed-warning
Merge pull request ocornut#346 from mgerhardy/280
Merge pull request ocornut#345 from mgerhardy/322
Merge pull request ocornut#344 from rherilier/fix-gcc-warnings
Merge pull request ocornut#336 from rherilier/add-isusingviewmanipulate
Merge pull request ocornut#335 from RedSkittleFox/alternative_window
Merge pull request ocornut#334 from ocornut/fix-beginchildframe
dear imgui update and small fixes
Merge pull request ocornut#316 from Batres3/2DSupport
Merge pull request ocornut#326 from Sayama3/use-push-pop-id
Merge pull request ocornut#328 from georgeto/master
Merge pull request ocornut#330 from maritim/master
Merge pull request ocornut#331 from GiovanyH/patch-1
Merge pull request ocornut#318 from dougbinks/imgui_math_operators
Merge pull request ocornut#312 from kimidaisuki22/master
div 0 fixed
Merge pull request ocornut#301 from ZingBallyhoo/using-any
Merge pull request ocornut#300 from Clog41200/Configurable-limits
Merge pull request ocornut#298 from xDUDSSx/fix/rotation_circles
Merge pull request ocornut#297 from xDUDSSx/fix/vertical-aspect-scaling
Merge pull request ocornut#289 from ComputationalBiomechanicsLab/fix_isusing-ignores-setid
Merge pull request ocornut#291 from ocornut/fix_math_operators_include
Merge pull request ocornut#282 from MohitSethi99/master
Merge pull request ocornut#276 from pthom/virtual_destructors
Merge pull request ocornut#271 from idbrii/clip-parent
Merge pull request ocornut#270 from idbrii/btn-behaviour
Merge pull request ocornut#265 from mgerhardy/pr/fix-minor-formatting
Merge pull request ocornut#264 from mgerhardy/pr/div0
Merge pull request ocornut#269 from peter1745/hatched-line-thickness-enhancement
Merge pull request ocornut#259 from miyanyan/master
Merge branch 'master' of https://github.com/CedricGuillemet/ImGuizmo
update dear imgui
Merge pull request ocornut#256 from Aidiakapi/patch-1
Merge pull request ocornut#252 from aaronkirkham/master
Merge pull request ocornut#249 from rokups/rk/mouse-capture
Merge pull request ocornut#246 from mgerhardy/pr/viewmanipulate
removed commented code
fix click view cube
Merge pull request ocornut#231 from mgerhardy/master
Merge pull request ocornut#230 from mgerhardy/master
Merge pull request ocornut#228 from longod/master
Merge pull request ocornut#227 from madeso/master
AddBezierCubic
Merge pull request ocornut#226 from sherief/master
revert culling test commit
Merge pull request ocornut#203 from rokups/rk/misc-fixes
Merge pull request ocornut#212 from zhaijialong/fix-behind-camera-cull
Merge pull request ocornut#209 from VictorFouquet/fix_normalize
scale is always local
Merge pull request ocornut#202 from pezy/master
imguizmo namespace
Merge pull request ocornut#194 from JonathanHiggs/vcpkg-example
1.84 WIP
Merge pull request ocornut#197 from idbrii/seq-btn-color
Merge pull request ocornut#196 from idbrii/seq-big-handles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants