Nevermind, I hadn't run all the tests...
Also in hindsight it's a bad idea to block all other user code in a with block on the library's side...

I think that this is an interesting idea and we shouldn't be so prompt in rejecting it. Don't worry about failing tests, 2.0 is still in dev, and breaking stuff is expected going to this new major release. Moreover, as far as I can see, tests that broke are those introduced in the with functionality which may need to be adapted with your PR.

I am a little bit more concerned with performances, it seems to me that this adds a lot of work on top of simple function calls. However, this is already the case for the with functionality and I am far from having a deep understanding of those things in python, so it may not be a problem. Maybe we should test those things.

Also in hindsight it's a bad idea to block all other user code in a with block on the library's side...

Can you develop? I am not sure to understand all implications of your proposal, I'll have to dig deeper into it to grasp a better understanding.

Anyway, thanks for the help :)

I'm glad you also think this is an interesting idea. My main concern, that I only realized after submitting the PR, is that this simply cuts off all the content of the with block, meaning that if the user has some custom logic inside it, it will be completely ignored. Perhaps we could make this feature togglable? Some food for thought...

Also about the tests, what put me off is that after realizing that above, then the failing tests that concerned me the most where mainly about catching exceptions inside the modified with statements, and since I already understand little about how the method to skip the with blocks works, I was intimidated to look into how exception traces work and then gave up a bit too quickly.

However I'm willing to look more into it since you also think this might be a good idea, so let me know about the first concern with skipping custom user logic.

PS: forgot to touch on performance. Sure this does impact performance, it has to instantiate a whole new class for everyone of those begin's since I couldn't manage to implement this directly in Cython, firstly because I am not experienced with it, and secondly because this uses some weird internal types for system traces that I'm not sure exist on the C side... If someone more acquainted with Cython is willing to look into this I'd be very grateful. However from my testing the overhead was not noticeable at all, but I didn't run any particular tests, I simply ran an example window and it reached the stable 60 fps without issues. I'll test properly when I have time.

I was thinking with my OOP brain and not with a logical train of thought... a wrapper class is completely overkill for this purpose, simple functions do the job just fine.

This way the logic for the with statements remains largely untouched, when using the 'old' style with if checks there are no issues with the class type, and now the .opened AttributeErrors are fixed. Down from 14 failed tests to 3, and those 3 all involve raising exceptions inside the with statement, will have to look deeper into it for those.

Performance wise I get fps between high 550s and low 560s fps with if-style code, while in the mid 550s using the with-style code. Looks to me like this with skip functionality does impact performance but in a veeery negligible manner.

Also about the 3 failing tests it's obvious: these tests check if an exception can be thrown from inside the with statement, but the exception will never be thrown in this case because the with block is skipped altogether (when .opened is False, and this is the case with the tests because there is no user to interact with the widgets). So these 3 tests will need to be reconsidered if we decide to go forward with this feature.

So the last concern that remains in my opinion is the same I had originally:

• should it be always enabled?
• should it be a module level toggle (imgui.skip_with_statements(True/False))?
• should it be a begin level toggle (imgui.begin_popup_modal("Example", skip_with=True/False))?
• if togglable, should it be enabled or disabled by default?
• should it even be part of the library?

I'm open to any kind of suggestion!

Thanks for the clarification and proposal!

My main concern, that I only realized after submitting the PR, is that this simply cuts off all the content of the with block, meaning that if the user has some custom logic inside it, it will be completely ignored.

Ok, I understand it now.

[...] simple functions do the job just fine

Indeed, this looks cleaner and seems to fix the previously raised issue. However I have some doubts about the global _orig_trace, it may only be a lack of understanding on my part, but wouldn't this break for nested skipped-with ? I may have to look deeper into it to be sure. Anyway, it starts to look like something which is great!

Concerning perfs, I am not sure that looking at FPS count is a great way to measure it. This is influenced by too many other things and I think that perf problems will show only for big projects which extensively use the functionality, i.e., with many calls by frame.

In my opinion, a better measurement technic is to isolate as much as possible some examples of use in small functions (similarly to test cases) and use timeit to compute the average computation time on 1000000 calls. I made a quick and dirty setup to do it and I measure taking your popup example:

Branch: dev/version-2.0
old style 'begin/end': 0.7116959999999999 µs
old style 'with': 0.9190690000000004 µs

Branch: skip-with-statements
old style 'begin/end': 0.7256831999999998 µs
old style 'with': 0.9228938000000007 µs
new 'withskip': 1.5783046999999994 µs

(I also quickly tested other types of begin/end)
(I didn't check if the output is correct in all cases)

This isn't by any means an extensive measurement and may need more rigor, but we can already note two things:

• Your change doesn't seems to impact the already existing setup which is good
• New withskip looks 1.5x slower than with and 2x slower than begin/end (note that with is also ~1.5x slower than begin/end)

This may be accelerated using cython instead of python, I'll need to dig deeper into that. For now, I am not sure if this behavior should be the default one. It is clearly more programmer-friendly but the penalty feels a bit harsh for such a used feature of the lib. Note that this also raises concerns with regard to the original with proposal. I will have to run some experiments and think about all of that.

Yes, it could be a togglable feature but wouldn't it also cost a set of conditional around each use of the feature? And wouldn't it be a bit confusing at use? I still think the whole thing is a good idea and I would prefer to have it as default or not have it at all, but we need to figure out if we can make it as efficient as possible. This pattern is used everywhere in imgui and if we don't take special care with it it will clumps up and bite us in the back.

Note: We not only need to consider the raw timing of function calls but also how many times we expect those calls to occur in a "normal" application in order to estimate the total penalty.

Note: I would be curious to compare perf on an old branch without the with functionality. Comparisons with begin/end may be biased...

Oh wow. Yea that is very slow, indeed FPS is a horrible way to test this. I'll try to see what can be done through Cython.

About the global and nested skipped-with, I too had that concern initially, that's partly why I originally went with a class wrapper, so it would be assigned to the class and not globally. However, if you think about it, when entering a with statement _orig_trace doesn't get set normally, while it does when a with gets skipped, but at that point, if there are more withs inside it, they will be ignored. Also it remains set very briefly, just to enter the with block, raise the exception, exit the block and ignore the exception.

I, too, think that this should either be default or not exist at all, and yes togglable would mean additional if checks and more complexity on user side. So yeah I'll see what can be done in terms of optimization, and if nothing can be done then we'll just have to let this go, no big deal.

About the global and nested skipped-with, I too had that concern initially, that's partly why I originally went with a class wrapper, so it would be assigned to the class and not globally. However, if you think about it, when entering a with statement _orig_trace doesn't get set normally, while it does when a with gets skipped, but at that point, if there are more withs inside it, they will be ignored. Also it remains set very briefly, just to enter the with block, raise the exception, exit the block and ignore the exception.

Mmmh yeah, you make a point here.

Oh wow. Yea that is very slow

To be fair, it is not that slow. For a 60fps target, we have a 16000µs budget per frame. I do not have an official number for the average number of call but it doesn't seem to be reasonable to expect ~1000 usage of the functionality per frame (note that the frame budget would probably be spent elsewhere in a typical application which lowers considerably this bound).

Moreover, since the other usages are conserved and since they do not imply a noticeable overhead, if someone needs to get every cycle he can he always has the possibility to use the begin/end syntax. I did quickly measure the perf before the introduction of the with functionality and I did not notice a particular difference in the performance of the original begin/end call.

If we can get some little more by going to cython it would be great, else I am still willing to include the functionality even in this state.

Ok so, bit of an update. I am yet try try porting to Cython, but I tried simplifying the code itself a bit:

The calls this makes to the inspect module are just wrappers around lower level sys module calls:

frame = inspect.currentframe().f_back
frame.f_trace = raise_skip_with_statement

can be replaced with:

sys._getframe(1).f_trace = raise_skip_with_statement

_settrace()'s only advantage over a standalone sys.settrace() is that it avoids some output to stderr when using pydev. I'm just going off the comments in conditional_context, so this might not be correct, but Google tells me that pydev is a plugin for python development with the Eclipse IDE, and also this code is 2 years old so things might have changed. Using simple sys.settrace() calls might save some processing.

From what I understand, sys.*trace() stuff manipulates the trace function, which allows to implement a debugger for python. On standard release python, the trace is None by default, but I'm guessing that when using the debugger in an IDE it would be set automatically. For the exception to be injected and raised inside the with statement, a trace function needs to be set, but it doesn't matter what it is set to, hence the check for None and the restore to previous value. To not break debugger support this can't really be simplified from what I can see. Perhaps using a real empty function instead of creating a lambda each time, and skipping the superfluous _orig_trace = None reset, but not much more.

Edit: not having the code you used for your tests I quickly made my own snippet, and applying the changes I outlined above my timings drop from 7.4 µs to 3.7 µs on laptop and from 2.4 µs to 1.1 µs on desktop, still running only inside python, no cython yet. This is the test I ran in case I'm not doing something right:

import timeit
import imgui
import pint

u = pint.UnitRegistry()
imgui.create_context()
io = imgui.get_io()
io.delta_time = 1.0 / 60.0
io.display_size = 300, 300
io.fonts.get_tex_data_as_rgba32()
io.fonts.add_font_default()
io.fonts.texture_id = 0

imgui.new_frame()
def popup():
    with imgui.begin_popup_modal(""):
        print("DID NOT SKIP")  # Never got printed
took = timeit.timeit(popup, number=1000000) * u.seconds
avg = took / 1000000
print(avg.to(u.microseconds))
imgui.render()
imgui.end_frame()

And update 2: can't manage to get it working in cython. It compiles fine, but even just copy pasting the same exact python code from _skipwith.py file into core.pyx, without adding any cdefs, gives this at runtime:

DID NOT SKIP
Traceback (most recent call last):
  File "/home/willyjl/Coding/test/test3.py", line 21, in <module>
    popup()
  File "/home/willyjl/Coding/test/test3.py", line 16, in popup
    with imgui.begin_popup_modal(""):
  File "imgui/core.pyx", line 5412, in imgui.core._BeginEndPopupModal.__exit__
    if self.opened:
  File "imgui/core.pyx", line 5412, in __exit__
    if self.opened:
TypeError: 'NoneType' object is not callable

Also tried making a new _skipwith.pyx and including it in core.pyx, but nope.

But do let me know your test results with the optimizations i made above, it might be enough to make a difference.

Edit: the issue appears to be setting the tracing function, as per cython/cython#1769

Ok, I've spent way too long on this and I can't get any further than this:

The issue with TypeError: 'NoneType' object is not callable was a compiler option, just needed to set # cython: linetrace=False for this particular file.

For some reason _raise_skip_with_statement doesn't want to work as cdef: giving it proper argument names it compiles but it gives more weird NoneType errors at runtime. Had to leave that as a normal def.

All the trace function stuff is doable in Cython without relying on the sys module, as can be seen here and here. However, it looks very complex, I can't wrap my heard around it and I've only gotten segmentation faults and compile errors so far.

Had to change sys._getframe(1), since with Cython we're on the C frame stack we're interested in sys._getframe(0).

Results are:

PyPi v1.4.1:
if begin/end: 0.212 µs

7cbbd46 (initial with support):
if begin/end: 0.224 µs
with + if:    0.342 µs

8010eed (initial skip with wrap):
if begin/end: 0.623 µs, sometimes broken
with + if:    3.114 µs
with + skip:  2.932 µs

d446049 (skip with simple functions):
if begin/end: 0.227 µs
with + if:    2.540 µs
with + skip:  2.428 µs

f815298 (skip with optimized):
if begin/end: 0.221 µs
with + if:    1.230 µs
with + skip:  1.141 µs

8bbe86b (skip with cythonized):
if begin/end: 0.208 µs
with + if:    0.914 µs
with + skip:  0.836 µs

Not sure why my ratios are so different from the ones you got in your tests... anyway that's the best I can get it to run.

Also sorry for blabbering on forever, I just like documenting and explaining what I do as a way of record keeping :D

Great job, thanks!

Not sure why my ratios are so different from the ones you got in your tests... anyway that's the best I can get it to run.

I did my test again using the same commits as you and I get the following results:

7cbbd46 (initial with support):
if begin/end:  0.6938666 µs
with + if:     0.8792884 µs

8010eed (initial skip with wrap):
if begin/end:  1.1295596 µs
with + if:     3.7716661999999994 µs
with + skip:   3.671653000000001 µs

d446049 (skip with simple functions):
if begin/end:  0.6896078999999999 µs
with + if:     3.3172569 µs
with + skip:   3.2572821000000003 µs

f815298 (skip with optimized):
if begin/end:  0.6898095 µs
with + if:     1.9095538000000005 µs
with + skip:   1.8357392999999997 µs

8bbe86b (skip with cythonized):
if begin/end:  0.6926675 µs
with + if:     1.6576 µs
with + skip:   1.6054598999999996 µs

Scale is different than yours due to different hardware and we don't measure exactly the same function. However, the ratios seem similar. Strangely, 'with+if' seems to do way worse than in my first experiments, I probably did something wrong back there, sorry.

Anyway, this is a huge improvement with respect to the original proposal, and a significant one with respect to your second attempt. In my opinion (but should be measured to be sure), the bottleneck is probably the python call to sys.gettrace/settrace. The next step would be to reimplement the tracer in cython as you proposed. I would have to check everything more in-depth and try to wrap my head around it.

Looking at sys documentation I saw this information on both gettrace and settrace:

CPython implementation detail: The settrace() function is intended only for implementing debuggers, profilers, coverage tools and the like. Its behavior is part of the implementation platform, rather than part of the language definition, and thus may not be available in all Python implementations.

This makes me a little bit worried. We may have to go to the full custom implementation in cython just to be sure to consistently have the functionality on every machine. What is your opinion about that? I'll try to study this option and see what I can do.

For some reason _raise_skip_with_statement doesn't want to work as cdef

I think this is because you are using _raise_skip_with_statement in the python context and thus it has to be defined as python. We could have cpdef it but I'm not convinced that this will leads to a significant improvement.

I have a few thoughts and a possible alternative which would solve this issue in a more pythonic manner.

CPython implementation detail: The settrace() function is intended only for implementing debuggers, profilers, coverage tools and the like. Its behavior is part of the implementation platform, rather than part of the language definition, and thus may not be available in all Python implementations.

I share the same concerns about using sys._gettrace. It seems that an implementation of this feature that does not use implementation specific features such as sys.gettrace/settrace would be preferable.

Here is a proof of concept of an alternative solution that I have devised.

from contextlib import contextmanager

class _ImGUIOpenError(Exception):
    pass

@contextmanager
def begin(dummy):
    try:
        yield dummy
    except _ImGUIOpenError:
        print("The inside of the with statement was skipped as opened == False")
    finally:
        # close this
        pass

@contextmanager
def require_opened(value):
    if not value.get("opened"):
        raise _ImGUIOpenError()

    yield value

    pass

with begin({"opened": True}) as ctx, require_opened(ctx):
    print("This will execute as expected because opened is True")

with begin({"opened": False}) as ctx, require_opened(ctx):
    print("This will never execute as because opened == False", ctx)

with begin({"opened": True}) as ctx:
    if ctx.get("opened", False):
        print("You can also use this context manager in this way")

with begin({"opened": True}) as ctx, require_opened(ctx):
    # Anything here will execute as expected
    raise Exception("This exception will be thrown as normal and exit the program")

The output of this above code would be as expected:

This will execute as expected because opened is True
The inside of the with statement was skipped as opened == False
You can also use this context manager in this way
Traceback (most recent call last):
  File "poc.py", line 37, in <module>
    raise Exception("This exception will be thrown as normal and exit the program")
Exception: This exception will be thrown as normal and exit the program

Note that the use of @contextlib.contextmanager was just a choice to make my example more concise. The documentation covers this helper decorator quite well. This can be replicated using traditional class based context managers (this may even be more performant).

I think such a solution would be preferred as there is no user code being skipped without being explicitly skipped as intended. There probably is a way to couple those two context managers together and have something like:

with imgui.strict_begin_popup_modal(""):
    # This would only happen popup.opened == True

All the names are just placeholders, there is probably a better name for such a function
I just threw together this proof of concept after reading the discussion. I am open to elaborate on any point and hear any feedback.

I think such a solution would be preferred as there is no user code being skipped without being explicitly skipped as intended. There probably is a way to couple those two context managers together

Agreed, this looks like the best compromise. Thanks, I'll look into implementing this properly tomorrow :D

There probably is a way to couple those two context managers together and have something like:

with imgui.strict_begin_popup_modal(""):
    # This would only happen popup.opened == True

Can't manage to set that up, the exception always propagates...

However the main proposal could be even simpler, I was thinking something like:

# core.pyx
class _ImGUIOpenError(Exception):
    pass

class require(object):
    def __init__(self, value):
        self.value = value

    def __enter__(self):
        if not self.value:
            raise _ImGUIOpenError()

    def __exit__(self, exc_type, exc_val, exc_tb):
        pass

class _BeginEndPopupModal(object):
    ...
    def __exit__(self, exc_type, exc_val, exc_tb):
        ...
        if exc_type is _ImGUIOpenError:
            return True

# userland
with imgui.begin_popup_modal("Modal Popup") as popup, imgui.require(popup.opened):
    imgui.text("I'm a popup!")

This would mean that this whole PR could boil down to just a single generic cython class that skips the with block on a simple bool condition.

Or we could have a skip function on the context itself, which might be more intuitive, and not to mention faster in perf terms:

# core.pyx
class _ImGUIOpenError(Exception):
    pass

class _BeginEndPopupModal(object):
    ...
    def __exit__(self, exc_type, exc_val, exc_tb):
        ...
        if exc_type is _ImGUIOpenError:
            return True
    ...
    def skip_with_if_needed(self):
        if not self.opened:
            raise _ImGUIOpenError()
        

# userland
with imgui.begin_popup_modal("Modal Popup") as popup, popup.skip_with_if_needed():
    imgui.text("I'm a popup!")

(Of course both of those are still pure python, when we decide I'll convert to cython code.)

Personally I'm fine with both but I'd prefer the latter, but do let me know what you think!

Edit: I haven't done any extensive testing yet, but while on 8bbe86b (the last commit I pushed here, with cythonized withskip) I get 2.7 µs, with the skip_with_if_needed proposal above I get 1.7 µs and the original with functionality by mCoding takes 1.0 µs. Of course this means that it is not automatic, but it allows for more flexibility on the user's side, it's faster, and doesn't depend on cpython implementation details like get/settrace.

Hey @Willy-JL, @KinoxKlark, @apadhi20, and @mCodingLLC. I know I wasn't active here for long time but this is something I've been thinking about for a very long time.

I gave a shot to it after seeing James' video today without knowing any latest changes in dev branch and or even this PR. By the way, good job on #255 and video explainer @mCodingLLC!

Long story short, I came to conclusion that the easiest way would be to have extra function in imgui core and/or method in the _BeginEndX context manages. I know that you all have those pieces already figured out so won't suggest implementation details. I'm just giving you confirmation about right API direction. Just want to give suggestion about naming.

In my opinion the most expressive way for those context would be as in following example:

with imgui.begin_popup_modal("Modal Popup") as popup, imgui.only_if(popup.opened):
    ...

Another approach could be having a breakout method method on those context tuples (let's call them so :P):

with imgui.begin_popup_modal("Modal Popup") as popup:
     imgui.breakout_if(not popup.opened)

I'm not really sure about the second option and its naming (maybe break?, maybe without argument?) but I'm pretty sure you can all figure it out. Also, I can envision situation when this could be more readable to avoid long with statement. That's why it may be reasonable to have both of them (as experimental features?) and see what is preferred by users. Anyway, I'm just giving ideas and leaving decision to current maintainers.

Thanks for the input, and glad to see you're still interested in pyimgui @swistakm!

Actually that's exactly what was missing in my opinion, some proper naming! I personally feel like the "only if" paradigm fits perfectly in this situation. However having a module wide function for that I feel like could be counter intuitive, and could lead curious users to using it wrongly and be left wondering what on earth a _ImGUIOpenError is xD. What I propose instead is (and I kinda feel like you mentioned it yourself with "a breakout method method on those context tuples" but that example under it seems unrelated imo):

with imgui.begin_popup_modal("Modal Popup") as popup, popup.only_if_open():
    ...

With this option the "skipping with blocks" is entirely bound to their relative begin/open functions and context tuples, so in my opinion it feels more natural and makes it clear that they should only be used in this particular use case.

Also, I can envision situation when this could be more readable to avoid long with statement. That's why it may be reasonable to have both of them (as experimental features?) and see what is preferred by users. Anyway, I'm just giving ideas and leaving decision to current maintainers.

Well how this works is that the only_if_x() functions would raise an (internal to imgui) exception, for example the _ImGUIOpenError I mentioned before, just as a way to break outside of the with block, and then the __exit__ method of the begin/end "context tuples" would ignore the exception if is of type _ImGUIOpenError. This means that it doesn't really matter where the user uses the only_if_x() call, as long as it is after opening/beginning the widget and inside the with block. What I'm trying to say is that the example usage above, and this:

with imgui.begin_popup_modal("Modal Popup") as popup:
    popup.only_if_open()
    ...

would work exactly in the same way, out of the box, with no additional code required.

Personally I feel like this is the most natural solution to implementing this feature, but do let me know what you all think about this.

EDIT: I have updated the OP for this issue with the new status quo and implemented it the way I described above. I am yet to test the timings I get with the new changes

Just ran some tests on 6ae5768:

if begin/end:         0.151 µs
with/if opened:       0.308 µs
with/only if:         0.583 µs
with/only if newline: 0.581 µs

Just so we're on the same page, this is how I got these results:

import timeit
import imgui
import pint

u = pint.UnitRegistry()
imgui.create_context()
io = imgui.get_io()
io.delta_time = 1.0 / 60.0
io.display_size = 300, 300
io.fonts.get_tex_data_as_rgba32()
io.fonts.add_font_default()
io.fonts.texture_id = 0

def if_beginend_popup():
    if imgui.begin_popup("Example"):
        imgui.text("Example")
        imgui.end_popup()
def with_if_opened():
    with imgui.begin_popup("Example") as popup:
        if popup.opened:
            imgui.text("Example")
def with_only_if():
    with imgui.begin_popup("Example") as popup, popup.only_if_open():
        imgui.text("Example")
def with_only_if_newline():
    with imgui.begin_popup("Example") as popup:
        popup.only_if_open()
        imgui.text("Example")

imgui.new_frame()
average = lambda func: (timeit.timeit(func, number=100000)*u.seconds / 100000).to(u.microseconds).magnitude
print(f"if begin/end:         {average(if_beginend_popup):.3f} µs")
print(f"with/if opened:       {average(with_if_opened):.3f} µs")
print(f"with/only if:         {average(with_only_if):.3f} µs")
print(f"with/only if newline: {average(with_only_if_newline):.3f} µs")
imgui.render()
imgui.end_frame()

Can't say that I'm over the moon with these results, but also this might be the best we can get without making it the default behavior... and in the grand scheme of things it's not the end of the world for a third of a microsecond...

Let me know your thoughts