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

add a directory watcher to file watcher #27

Open
wants to merge 1 commit into
base: wip
Choose a base branch
from

Conversation

underdoeg
Copy link
Contributor

Add directory watching features to le_file_watch.

The API is a little bit of a mess for backwards compatibility. It all started with the problem that I wanted to have the Event enum within le_file_watcher_api. But then the directory settings had to be in there as well to have a clean access to Event. So file settings also had to move there to be consistent. thus the alias was born to not break anything.

There are also better ways to solve the implementation, but this should work, is hidden and fast enough.

no windows implementation yet though...

@tgfrerer
Copy link
Owner

Hmm. this might need a bit more work - especially to make it work cross-platform.

The main issue I see is that exposing an internal API externally will increases the number of things that have to be maintained and safeguarded against accidental misuse...

There are a couple of potential sharp corners here: if the API uses a callback-based method, this will blow up when the module containing the callback code gets hot-reloaded. There is a mechanism in Island to work around this, and I've written extensively about it, but it's a Linux- and x64 specific solution, and won't work on Windows, for example.

Let's think about this for a little bit :)

@underdoeg
Copy link
Contributor Author

underdoeg commented Jan 21, 2021

Yeah platform specific code is always a burden, I know. Would be solvable in this case though because it is pretty straightforward.

What do you mean by "exposing an internal API externally "? My implementation does not really expose more than the previous one. Or was le_file_watcher never meant to be used in third party application code?

[EDIT] Ah gotcha, the callback gets hot reloaded and thus the function pointer is invalid. So all the shader reloading stuff is not properly working on windows ATM?

@underdoeg
Copy link
Contributor Author

So a less headache inducing strategy would be to return a list of modified files and directories in the poll function?

struct FileWatcherEvent{
    Event event; // File or directory modified, removed, etc...
    const char* path;
}

// somewhere in update
std::vector<FileWatcherEvent> events = le_file_watcher.poll();

// do something

@tgfrerer
Copy link
Owner

tgfrerer commented Jan 21, 2021

shader hot-reloading and any other hot-reloading works as expected on Windows, but there, hot-reloading the le_window module after you changed it for some reason, for example, is tempting the gods.

windows file-watching is- as far as i remember directory-based in the first place, so this part of the api should be pretty straightforward to port, but then windows has a different set of kernel messages to linux which get triggered on directory change, and we'd need to find a meaningful subset...

yes, the file watcher was bashfully hidden away, it looks like this flushes it out into the open ;) I'm not totally against exposing it, but I'd rather we do it in a way that tries to design away sharp api edges, which is why i would suggest to ponder on it a little bit more :)

[EDIT] yes, something like poll could be a nice way to deal with not wanting to have callbacks exposed!

@underdoeg
Copy link
Contributor Author

It was bound to be dragged into the open as soon as you talk about hot reloading code, you are already hot reloading any kind of asset in your mind ;)

About the subset of events. I have worked with this cross platform library in the past: https://github.com/ThomasMonkman/filewatch and took a simplified version of the events you can find there. so I think create, delete, edit and move should be safe

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.

2 participants