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

Provide user-defined callbacks for file operations #923

Closed
ldecarufel opened this issue Nov 29, 2016 · 14 comments
Closed

Provide user-defined callbacks for file operations #923

ldecarufel opened this issue Nov 29, 2016 · 14 comments
Milestone

Comments

@ldecarufel
Copy link

ImGui currently uses standard library function to read/write to files. This works most of the time, but it would be useful to be able to provide our own implementation of those functions through callbacks.

In a multithreaded game engine, for example, it would be better to perform the save operations in a background thread.

Also, a game engine can implement it's own file manager, but it is currently not possible to make ImGui use it.

@ocornut
Copy link
Owner

ocornut commented Nov 29, 2016

The only file operation that may happen past initialisation is saving the .ini file. Do you really need that to go through an asynchronous system?

I don't know of a game hardware that can run ImGui where fopen()/fread() doesn't work given the right filename. Do you have something in mind?

@ocornut
Copy link
Owner

ocornut commented Nov 29, 2016

PS: Not trying to dodge the request but we've got a million todos and although I can see the desire for that I need to assert that it is insert a strict blocker to not have those things available.

If periodic blocking write of the .ini file are indeed a problem, we could e.g. provide a function to explicitly trigger .ini file writing which would serve as a workaround that is trivial to implement. For development/tooling my intuition is that an occasional xx ms spike while saving to disk isn't a real issue.

@ratchetfreak
Copy link

IMO making sure that every function that requires reading a file has an alternative that can just take a blob of memory as the file's contents is fine. (I believe that is already the case)

Then maybe a callback for when the file needs to be written out. (the default will just write the .ini file) which would allow user code to copy to a local buffer (if necessary) and then dispatch a async file write.

@ocornut
Copy link
Owner

ocornut commented Nov 29, 2016

I agree about this in theory. In practice I need to manage my time along with the scope/complexity/maintainability of this project and I wonder if asynchronous saving of the .ini file is really a real world problem that needs fixing.

@ldecarufel
Copy link
Author

It's mostly the continuous saving that's potentially problematic. I'm on a multi-threaded engine and I'd like to avoid writing to disk inside the ImGui update. I'd prefer to cache the data in a memory buffer and save it asynchronously in another thread.

On Windows the write operation is probably cached in memory, but on PS4 or XboxOne it may be written to disk right away.

Anyway, this is not a blocking issue right now, but I thought it would help make ImGui more portable!

@ocornut
Copy link
Owner

ocornut commented Nov 29, 2016

Some notes:

  • The writing interval can be adjusted by setting io.IniSavingRate and typically it only happens if a window has been moved and the timer is continuously reset while moving. So with default settings, after a series of windows manipulations it would write the positions to disk 5 seconds after the last once, and only once.

  • You can set io.IniFilename to NULL to disable .ini writing. So a mediocre workaround if you still need .ini but want to disable writing during gameplay is to change this variable at rnutime.

  • Having just looked at the code now, I have fixed a glaring bug that would mark settings as dirty when clicking on a window's void but not moving the mouse. As I add functions to dump the .ini file to memory I might as well compare hashes before attempting any disk write.

  • At least on PS4 with file writes hosted on the remote PC I haven't had noticeable issue but I appreciate this may differ with different setup and you are correct it should be improved.

  • FYI The comments next to those functions are dated from the day I hastily wrote them five seconds before releasing the first imgui version publicly, and they say:

// Zero-tolerance, poor-man .ini parsing
// FIXME: Write something less rubbish

Which probably means they need some love :)
Will look at it!

@JWki
Copy link

JWki commented Jan 13, 2017

I was about to submit the same issue but also complement it with a possible pull request - I've recently had to add a callback mechanism to the built in serialization system in order to be able to write imgui layout to a cookie when running in an emscripten compiled web app. My implementation allows to register two callbacks in the IO structure: One to control where the .ini is read from, and one to control how to write it.
This works quite well for me and I could imagine it working well for others, if there's interest I can clean up my implementation and prepare a pull request.

@ocornut
Copy link
Owner

ocornut commented Jan 14, 2017

I will implement a solution for it. If you have some code feel free to post the patch/PR for reference/ideas only if that is trivial for you to do. I'm not sure adding new callbacks in the IO structure is the best idea unless you actually need to override fopen() also during initialization for the FONTS, but I yet have to hear a request for that. My suggested solution would be to make Settings related functions public to allow to access raw buffers, and provide optional hints/info to the user about when to save it perhaps.

@thedmd
Copy link
Contributor

thedmd commented Jan 22, 2017

I will share my solution with you. I added callback for saving and loading settings from user context.

struct ImGuiIO
{
    ...

    // Optional: save or load configuration.
    void        (*SaveIniCb)(const char* buffer, size_t size);
    size_t      (*LoadIniCb)(char* buffer, size_t size);

I imported changes from my local copy to settings-callbacks branch.

Example usage:

    static std::string mySettings;

    ImGuiIO& io = ImGui::GetIO();
    io.SaveIniCb = [](const char* buffer, size_t size)
    {
        mySettings.assign(buffer, size);
    };

    io.LoadIniCb = [](char* buffer, size_t size) -> size_t
    {
        if (buffer)
            memcpy(buffer, mySettings.data(), size);
        return mySettings.size();
    };

Save callback is straightforward, it receive a text buffer and size to save.
Load callback is called twice, first time with buffer set to NULL to determine size, second time with a buffer to read content.

Edit:
If there is an interest in this solution I will create PR compatible with upstream master branch. Please let me know.

@ocornut
Copy link
Owner

ocornut commented Jan 22, 2017

Thanks @thedmd!
I can see the branch no need to make a PR right now! As discussed above the only different I'm thinking of is to avoid adding callbacks and just make those explicit somehow? (User can call a func load settings / save settings + somehow figure out the dirty state).

@ocornut
Copy link
Owner

ocornut commented Jan 22, 2017

(It might be useful for others interested in this to make a PR actually, as I likely won't tackle this, however simple it is, until I finish shipping my game).

@thedmd
Copy link
Contributor

thedmd commented Jan 22, 2017

You're welcome. I will prepare a PR link it there shortly.

I want to minimize number of modifications I make in ImGui code. Callback seems to be simpler than introducing some kind of string management and got the job done for me.

I thought about exposing Load/Save function in public API. Settings loading is a part of initialization and I didn't wanted to change internals. Callback avoid problems like calling Load in a middle of the frame, resolving conflicts or duplicates. A function to queue settings save/load at the end of the frame however can be convenient.

@thedmd
Copy link
Contributor

thedmd commented Jan 22, 2017

#993 there is a PR

I had to remove 'ToDisk' suffix from function name and filename parameter because they didn't make sense in context of changes.

@ocornut ocornut added this to the v1.53 milestone Dec 12, 2017
@ocornut ocornut modified the milestones: v1.60, v1.61 Apr 7, 2018
ocornut added a commit that referenced this issue May 7, 2018
…), SaveIniSettingsToDisk(), SaveIniSettingsToMemory(), io.WantSaveIniSettings. (#923, #993)
@ocornut
Copy link
Owner

ocornut commented May 7, 2018

New API

// Settings/.Ini Utilities
// The disk functions are automatically called if io.IniFilename != NULL (default is "imgui.ini"). 
// Set io.IniFilename to NULL to load/save manually. Read io.WantSaveIniSettings description about handling .ini saving manually.
void          LoadIniSettingsFromDisk(const char* ini_filename);                  // call after CreateContext() and before the first call to NewFrame(). NewFrame() automatically calls LoadIniSettingsFromDisk(io.IniFilename).
void          LoadIniSettingsFromMemory(const char* ini_data, size_t ini_size=0); // call after CreateContext() and before the first call to NewFrame() to provide .ini data from your own data source.
void          SaveIniSettingsToDisk(const char* ini_filename);
const char*   SaveIniSettingsToMemory(size_t* out_ini_size = NULL);               // return a zero-terminated string with the .ini data which you can save by your own mean. call when io.WantSaveIniSettings is set, then save data by your own mean and clear io.WantSaveIniSettings. 

The disk function are not really necessary to expose, but I think it makes the API more consistent and obvious to have them (they already existed in the private space). The Disk functions are automatically called when io.IniFilename != NULL.

If you want to save manually, you can poll the io.WantSaveIniSettings flag:

bool   WantSaveIniSettings;  // When manual .ini load/save is active (io.IniFilename == NULL), this will be set to notify your application that you can call SaveIniSettingsToMemory() and save yourself. IMPORTANT: You need to clear io.WantSaveIniSettings yourself.

This should hopefully solve all the discussed use cases (and without callbacks :)
@ldecarufel @thedmd @JWki @ratchetfreak If you have time to check this out let me know if it works for you. Thanks!

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

No branches or pull requests

5 participants