Skip to content

Conversation

lubieowoce
Copy link
Contributor

@lubieowoce lubieowoce commented Oct 18, 2018

Edit: now that I look at it, this is a huge PR... if you'd like me to break it up into smaller ones, please let me know.

Fixes:

  • Fixed swapped parameters to Vec4 in multiple places (1cf1e86)
  • Fixed a division by 0 crash in integrations/glfw (173d8bb)

Additions:

Drawlists:

DrawList:
    add_line
    add_rect
    channels_split
    channels_merge
    channels_set_current

get_overlay_draw_list

Styling:

get_style

GuiStyle:
    from_ref
    color

Cursor functions:

get_cursor_pos
set_cursor_pos
set_cursor_screen_pos

Mouse functions:

is_mouse_down
is_mouse_clicked
is_mouse_released
get_mouse_pos

Misc:

plot_lines
get_time

In addition, I added some aliases. ImGui always shortens "position" to "pos", but we have both get_window_position() and get_cursor_pos(). It would be nice to settle on one and use it consistently. In the meantime, I added aliases from *_pos to *_position. (998d03e, b5b165a)

@lubieowoce lubieowoce changed the title Additions + a few fixes New functions/methods + some fixes Oct 18, 2018
@coveralls
Copy link

coveralls commented Oct 18, 2018

Coverage Status

Coverage decreased (-0.4%) to 46.49% when pulling 88f76c0 on lubieowoce:patch-1 into 43938a1 on swistakm:master.

@lubieowoce
Copy link
Contributor Author

Weird, looks like travis crashed with some virtualenv error before running anything on Python 2.7.

@swistakm
Copy link
Member

Yes it fails on python 2.7 but it's completely unrelated. It's old issue and I'm fixing it right now.

@swistakm
Copy link
Member

I have added fix for Python 2.7 builds so this should be resolved now. It would be nice to rebase this PR.

Good point with this pos/position inconsistency. I would prefer to have "position" as default name for these Python bindings. Maybe it would be nice to have "pos" aliases marked somehow with warning that this is an alias but it isn't that critical.

There is still ongoing problem with builds on Windows. They are hanging in queued state without any reason. I have submitted issue to Appveyor but if we won't get any fix we may try to use Windows builds on Travis as they are now available. That would greatly simplify the build pipeline.

ImGui consistently uses ImVec4's fields in the `x, y, z, w` order. Using  `x, y, w, z` instead caused bugs – for example, converting an "rgba" Vec4 to ImGui's internal color format would swap the blue and alpha components.
On Windows, when a window is minimized, GLFW reports a window size `w, h = (0, 0)`. Here, we're dividing by w and h, so upon minimization the app crashed from a division by zero.

NOTE: I'm not sure what `io.display_fb_scale` should be when the window is minimized, but since nothing is rendered, maybe we can leave it as it was before minimizing?
Also reformatted `add_rect_filled`
- get_cursor_pos
- set_cursor_pos
- set_cursor_screen_pos
ImGui always shortens "position" to "pos", but we have both `get_window_position` and `get_cursor_pos`. It would be nice to settle on one and use it consistently. In the meantime, I added aliases from `*_pos` to `*_position`.
- is_mouse_down
- is_mouse_clicked
- is_mouse_released
- get_mouse_pos + an alias `get_mouse_position` (see previous commit)
@lubieowoce
Copy link
Contributor Author

lubieowoce commented Oct 22, 2018

I rebased the branch. I prefer "position" as well, but I wanted to avoid messing up the PR with renames – I feel it's big enough as is.

@lubieowoce
Copy link
Contributor Author

lubieowoce commented Oct 23, 2018

@swistakm Btw, how is coverage measured? I didn't really see any tests, but I don't know how you'd test it anyway.

(I guess you could test some properties like "When you call imgui.set_next_window_size(w, h) and create a window, imgui.get_window_size() == (w, h)" but that'd cover only a small subset of the desired properties. One way of testing the graphical stuff would be to write "the same program" using imgui (C++) and pyimgui (python) and then check if they result in the same frame-by-frame output, but you'd have to simulate user input somehow.)

@swistakm
Copy link
Member

We don’t have any explicit tests yet but we have some usage examples in docstrings that are used to generate visual examples in the dosctrings. So there is a small pytest plugin that uses autodoc feature of Sphinx to generate tests out of these examples. That’s how we measure coverage. It’s rather doc-coverage than real test-coverage.

I know that’s kinda kludgy but it works. I was even thinking about making some reference images so we could compare these “tests results” with some reference images but I didn’t researched yet how we could do off-screen rendering on Travis/Appveyor.

@swistakm
Copy link
Member

I’m merging it. There are still some issues on Appveyor builds but these should be now fine as I have merged some CI fixes just recently.

@swistakm swistakm closed this Oct 23, 2018
@swistakm swistakm reopened this Oct 23, 2018
@swistakm swistakm merged commit 41112c5 into pyimgui:master Oct 23, 2018
@swistakm swistakm added the release pending Merged but still needs official release label Oct 23, 2018
@lubieowoce
Copy link
Contributor Author

lubieowoce commented Oct 23, 2018

how we could do off-screen rendering on Travis/Appveyor

This should be doable in OpenGL – you just draw to a buffer, and you're free to copy it and save to disk or something. I don't know what the environment is like in Travis/AppVeyor though – if it's all headless and there's there's no GUI shell/WM present, getting an OpenGL rendering context could be hard/impossible. (but I might be wrong about that – I don't have a lot of experience with headless servers)

@lubieowoce
Copy link
Contributor Author

This Stackoverflow answer talks about testing a GUI app on Travis.

The linked repo's travis.yml sets up a virtual screen with a DISPLAY env var. It also references this link: Travis CI: GUI and headless browsers which sounds relevant.

@swistakm
Copy link
Member

Yeah I know. This is already done that way for documentation purposes with FBOs although I create hidden window with glfw because it’s the easiest way to create OpenGL context. Besides context we need actual OpenGL implementation. I doubt Travis workers have any graphics hardware. I did already some research in past and it seems that this could be done with software driver like one available in Mesa 3D.

@swistakm
Copy link
Member

Oh didn’t see your earlier post. I will look into these links.

@swistakm swistakm added discussion and removed release pending Merged but still needs official release labels May 19, 2019
@swistakm
Copy link
Member

Released as 1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants