Skip to content

Conversation

supudo
Copy link

@supudo supudo commented Mar 10, 2017

Hey :),

This one contains the InputXYZ functions. There's one not-so-minor issue, which also is present in the ImGui demo window.
For InputText if the buffer size is not specified as something big like 1024 or bigger it will take the passed value and use it as a limit - so if you pass "Text" it will display text but will also put limit of 4 chars on the input. No idea why this happens, so i've put a 2056 char array as limit for now. Same applies for the InputTextMultiline function.
Another issue with the inputs i noticed is that it works too fast, meaning if i type "A" it's ok, but if i do backspace (delete) and enter (new lines) it deletes multiple characters/words or puts lots of new lines. Same behaviour exists in the ImGui demo window by the way, so i think it might be something either in the implementation or in Cython itself.

Will keep digging to see if i can fix it.

P.S. Same type of issues i noticed with scrolling - if you have a big height window and you scroll with the mouse for ex., it always goes either at the top or at the bottom, you can't position it in the middle. Same issues with the combo lists.

P.P.S. All of those are observed on Python 3.5 with the PySDL implementation, so they might not be present in Python 2.X.

Cheers,
S.

@swistakm
Copy link
Member

I will pull it locally to see what may be the cause if these issues.

If something does not work with imgui demo window it is either a bug in imgui C++ implementation or a problem with the inputs processing (so PySDL/GLFW integration). I will review the #14 PySLD2 PR soon and then compare results between different integrations.

@swistakm swistakm added this to the 0.0.1 - initial release milestone Mar 13, 2017
Copy link
Member

@swistakm swistakm left a comment

Choose a reason for hiding this comment

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

Text input functions require some changes but the float/input functions seem correct.

imgui/core.pyx Outdated
# return changed, bytes_text.decode('utf-8')

cdef bytes bytes_text = _bytes(value)
cdef char inout_text[2056]
Copy link
Member

Choose a reason for hiding this comment

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

This inout_text buffer size should be a function argument. This will require dynamic allocation and proper string type handling via Python/C API.

imgui/core.pyx Outdated

cdef bytes bytes_text = _bytes(value)
cdef char inout_text[2056]
strcpy(inout_text, bytes_text)
Copy link
Member

Choose a reason for hiding this comment

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

Could be simply strcpy(inout_text, _bytes(value)). The local bytes_text variable is redundant (see comments below).

imgui/core.pyx Outdated
_bytes(label), inout_text, sizeof(inout_text), flags, NULL, NULL
)

return changed, bytes_text.decode('utf-8')
Copy link
Member

Choose a reason for hiding this comment

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

We need to return a buffer, so inout_text instead of bytes_text. Also I'm not sure about this decode('utf-8') this introduces a compatibility issue in Python2.7.

In Python2.7 this function will accept str as value but returns unicode. We need to either force str somehow. Maybe right now for consistency we need some opposite implementation of the _bytes() function. Maybe _from_bytes()?

imgui/core.pyx Outdated
text_val = 'Please, type the coefficient here.'
imgui.begin("Example: text input")
changed, text_val = imgui.input_text('Amount:', text_val)
Copy link
Member

Choose a reason for hiding this comment

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

Label will be displayed on right so semicolon looks weird in the example.
You can either use hidden label with ## prefix:

changed, text_val = imgui.input_text('##Amount:', text_val)

or simply use imgui.same_line().

imgui/core.pyx Outdated
)
"""

# return changed, bytes_text.decode('utf-8')
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary comment

.. _guide-inputtext-flags:

Using input text flags
==================
Copy link
Member

Choose a reason for hiding this comment

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

Make the underline same width as the title. Otherwise Sphinx will generate warnings.

float width=0,
float height=0,
cimgui.ImGuiInputTextFlags flags=0
):
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as for input_text() function apply also here.

@swistakm
Copy link
Member

swistakm commented Mar 14, 2017

I did run this in my local environment and here is my feedback:

  1. The buffer size unfortunately must be set explicitly right now as a function parameter. Imgui expects that user knows how much of input he wants to process. Setting this to arbitrarily high value is not a proper solution. I would rather like to not have a default value for such param either because if we implement Allow for easy way to handle inout arguments #8 fully, this will have to be a keyword-only argument.
  2. Remember that buffer size is in bytes and the input is in unicode. This means that buffer size does not equal the number of characters. Function docstring should have a clear note on that.
  3. I have tried to run example code in both GLFW2 implementation and PySDL2. I have noticed the "multiple backspace and newline characters" issue only on GLFW3 and Python 3. I don't know what is the problem yet. But since it happens also in the demo window it is just implementation-specific issue or internal imgui problem. Will have to try raw C++ example from imgui sources.
  4. I have found that PySDL2 does not handle key modifiers correctly. Pressing SHIFT, CTRL, ALT etc. yields semi-random unicode code points.
  5. Callbacks are missing but they are optional so I can add support for them in the future.

3., 4. and 5. can be handled in separate issues/PRs.

@swistakm
Copy link
Member

I have found the reason for problem 3. It is a timing issue and both PySDL2 and GLFW3 do not measure time deltas properly. Will be very easy to fix.

@supudo
Copy link
Author

supudo commented Mar 15, 2017

Hey :),

All those PR got me confused and i won't be surprised if i messed the branches again.
Anyway, i've added the _from_bytes() (think it's correct) and fixed the input_text methods to be with length parameters.

Cheers,
S.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5c221ac on supudo:InputBoxes into ** on swistakm:master**.

@swistakm
Copy link
Member

swistakm commented Mar 15, 2017

This branch is fine. You did not mess anything here :). The _from_bytes() function looks great. It is something that I exactly had on my mind.

There are just some two issues with input_float3 and input_float4 functions to fix. You can find details in Appveyor build logs. Seems like creating this tests pipeline did finally pay out 🍾

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a89139f on supudo:InputBoxes into ** on swistakm:master**.

@supudo
Copy link
Author

supudo commented Mar 15, 2017

Hey,

Fixed and passed :)

Cheers,
S.

@swistakm swistakm merged commit ff4b918 into pyimgui:master Mar 16, 2017
swistakm pushed a commit that referenced this pull request Mar 16, 2017
swistakm added a commit that referenced this pull request Mar 17, 2017
* move common OpenGL code to separate class
* fix text input issues in PySDL2 integration (fix #29)
* fix scrolling issue in PySDL2 integration (refs #24)
potocpav pushed a commit to potocpav/pyimgui that referenced this pull request Oct 17, 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.

3 participants