Skip to content

Conversation

602p
Copy link

@602p 602p commented Apr 22, 2018

This adds functions to expose the PushID and PopID capability in imgui for manipulating the widget id stack. This allows you to elegantly have multiple widgets with the same implicit ids (usually their name.)

It also adds a simple context manager to the extra module that automates the pushing and popping of scope ids, and allows use without manually specifying an object to be the scope id (by creating an anonymous object for this purpose.)

Not sure what the conclusion was on #33 , but I placed the context manager in the extra module per discussion.

This closes #56 , assuming it is to @swistakm 's liking.

@coveralls
Copy link

coveralls commented Apr 22, 2018

Coverage Status

Coverage decreased (-0.2%) to 42.295% when pulling 4faa959 on 602p:master into 427ead5 on swistakm:master.

@602p
Copy link
Author

602p commented Apr 22, 2018

I've removed the behavior of the context manager where it would create an arbitrary object to be the scope. After running my project against the patch it seems like that dosen't work for all cases. It appears to be important that the scope object is the same between frames for things that have state managed by imgui (e.g. collapsed state of a treenode.) That is, if you have multiple imgui.tree_nodes with the same name, they all open/close together. If you give them all different scopes but use the same scope objects for each one each frame, it works perfectly. If you give them different scopes but change the scopes every frame then they will not respond to clicks at all (because imgui can't keep track of which is which with the different scopes.) For simple cases like keeping buttons and checkboxes from colliding with each other, a random scope seems to be fine.

To go with this, I've fixed it so that when you call push_id with an integer it pushes the value (as opposed to the address,) and when used with a string it similarly uses the imgui variant that will internally check string equality. This should make it behave as expected across frames for people using integers or strings as contexts. The implementation for pushing an integer is kinda odd, because directly casting to int and using the PushID(int) overload won't work if the int is larger than a C int. So instead it casts to void* via uintptr_t, which is ugly but actually what imgui does internally.

It's up for debate as far as what we want to offer in the context manager. Since creating an new object for the scope each frame only does what you want some of the time, it seems dangerous to have (at least as a default.) Therefore I removed that and it's almost exactly what was suggested in the issue. Maybe we want some utility class that is a context manager that pushes itself as the scope, thereby allowing you to keep a reference to it across frames? Here is a quick recipe for that:

class Scope:
    def __enter__(self):
        imgui.push_id(self)

    def __exit__(self, *args):
        imgui.pop_id()

#used like

s=Scope()

#and then each frame in your ui code
with s:
    imgui.button('abc')

I'm not sure if that's useful enough to merit inclusion in extra (at least, I don't think I'd find myself using that over using the piece of data I am constructing the UI for as my scope.)

@swistakm
Copy link
Member

Your observations about scopes are very good so it nice you have captured them in the docs. I will just have to take closer look at this case with int but generally the PR looks very nice. Also I have few comments related to docs and style so if you have spare time you can fix them in the meantime.

About the mentioned Scope() utility from the extra recipe. I think we can live without that. It is also risky because less experienced developers may use it wrongly (create at every frame as you have already mentioned) and this will be very confusing.

Thanks for contribution!

"""
core.push_id(arg)
yield
core.pop_id()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing newline at the end of file

the same object (is-equality in python) unless it is an integer or string.
Consider using the :func:`scope` context manager instead to manage the
push_id and pop_id pair.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add Args: and .. wraps:: sections as in other functions? These sections are later properly processed with Sphinx for documentation purposes.

The tree_node function can be a good example for .. wraps:: because it similarly wraps multiple oveloaded calls.

"""Pop an id scope.
Undoes the work of :func:`push_id`.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, .. wraps::

Undoes the work of :func:`push_id`.
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary newline

cimgui.PushID(_bytes(scope_id))
else:
cimgui.PushID(<void*>scope_id)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have always two newlines between every top-level function/class definition

Consider using the :func:`scope` context manager instead to manage the
push_id and pop_id pair.
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary newline


@contextmanager
def scope(arg):
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could describe it without mentioning pushes and pops? E.g. "Wrap code block with isolated scope for widget identifiers."

And maybe later just quicly describe that this may be useful if multliple widgets within the same window need to have same ID with some usage example.

@traverseda
Copy link

Any progress on this? I'd fine it useful.

@swistakm
Copy link
Member

I will take another look at this MR next week

@gingerBill
Copy link

Will this be added soon to the latest release?

@swistakm
Copy link
Member

I'm closing this because alternative implementation has been already merged.

@swistakm swistakm closed this Jul 20, 2020
@swistakm swistakm added the release pending Merged but still needs official release label Jul 20, 2020
@swistakm
Copy link
Member

Feature released in 1.2.0

@swistakm swistakm removed the release pending Merged but still needs official release label Jul 20, 2020
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

Successfully merging this pull request may close these issues.

How to handle multiple widgets with same "implicit" ID
5 participants