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

Fix clipboard crash and add extra methods #23

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pkhead
Copy link

@pkhead pkhead commented Jun 2, 2024

This pull request fixes a potential crash when ImGui interacts with the user's clipboard caused by the clipboard delegates being garbage-collected. It also adds extra methods that may be useful, namely one for setting the ini filename and several for drawing Image and ImageButtons with a tint and/or custom source UVs.

Comment on lines 74 to 95
/// <summary>
/// Set the filename of the INI file that ImGui uses to store the window configuration.
/// It is set to "imgui.ini" by default.
///
/// Call this after rlImGui.Setup.
/// </summary>
/// <param name="iniFilename">The file path to store the ini file.</param>
public static void SetIniFilename(string iniFilename)
{
if (iniFilenameAlloc != 0)
Marshal.FreeHGlobal(iniFilenameAlloc);

byte[] nameBytes = System.Text.Encoding.ASCII.GetBytes(iniFilename + "\0");
iniFilenameAlloc = Marshal.AllocHGlobal(nameBytes.Length);
Marshal.Copy(nameBytes, 0, iniFilenameAlloc, nameBytes.Length);

unsafe
{
ImGui.GetIO().NativePtr->IniFilename = (byte*) iniFilenameAlloc;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I like this being internal to rlImGui. it's not raylib specific and doesn't really have anything to do with this backend. This code would be used on any C# imgui system.
I'd prefer to keep rlImGui just for things that deal with raylib and ImGui, not random imgui utilities, they should go into a separate and optional library.

Comment on lines +285 to +287
private static GetClipTextCallback getClipCallback = null!;
private static SetClipTextCallback setClipCallback = null!;

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this all that's needed for the crash fix?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I did make a test with the old code, my code, and a GC button. On the old code, the clipboard would work fine until I pressed the GC button, at which point copying or pasting from/to a text field would cause a hard crash on the program. With my new code, the clipboard still worked fine even after pressing the GC button.

Comment on lines 690 to 705
/// <summary>
/// Draw a texture as an image in an ImGui Context
/// Uses the current ImGui Cursor position and the full texture size.
/// </summary>
/// <param name="image">The raylib texture to draw</param>
/// <param name="tintColor">The color to tint the image with</param>
public static void Image(Texture2D image, Color tintColor)
{
ImGui.Image(
new IntPtr(image.Id),
new Vector2(image.Width, image.Height),
Vector2.Zero, Vector2.One,
new Vector4(tintColor.R / 255f, tintColor.G / 255f, tintColor.B / 255f, tintColor.A / 255f)
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to keep rlImGui-cs feature compatible with the C++ version.
I'm not against having a tint version, but it should not duplicate code like this.
The tint should be an optional argument on the existing functions, or the non-tint versions should call the tint versions with a default tint. This feature would also need to be added to the C++ version for parity.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, I have fixed the code duplication problem, and I'll begin working on a PR to the C++ version with the same features.

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.

None yet

2 participants