stewartmatheson / projectx

0 stars 1 forks source link

Asset watcher stuff #70

Closed msbit closed 3 years ago

msbit commented 3 years ago

Hey mate,

You've probably gotten pinged a bunch as I've been adding asset watcher PRs (#67, #68 and #69) but I thought I'd just comment here to centralise.

They're all pretty much standalone, but there will likely be issues of merging after the first one (maybe not, fingers crossed) so merge them as/if you see fit and I can rebase if necessary.

I've also got https://github.com/msbit/projectx/tree/asset-watcher-platform waiting in the wings, which splits out the win32 specific stuff and would allow different platform implementations to be added as you go. This shuffles files around a bit, and will need a rebase so I'll hold off on that until these ones settle down.

There are also likely some win32-isms that might make the watching a little cleaner (FindCloseChangeNotification/FindNextChangeNotification for example) plus some discussion about whether exiting if any of the watching setup fails is the best option (vs simply ending the thread and living without watching for that run).

👍🏻

stewartmatheson commented 3 years ago

Yoooooo Tom!

Think I got all the merging done. I'm going to try and finish off this refactor first (hopefully not too much longer) then I'll take the new master branch for a spin and get all of my changes rebased in. I don't think it's going to be too bad as I have not really touched asset watcher at all.

Just had a quick look at your platform branch. Nice one. I like how it's not too macro heavy. I find lots of macros can make C code hard to follow so yeah as soon as you are ready let's get that in.

Interesting point regarding the shutdown. Generally, I like to exit early but I guess we can put a console message up with the error. Not too bothered either way.

msbit commented 3 years ago

Nice!

There were a couple of small things that fell through the cracks, not super surprising given how many interdependent branches I threw at you :) I've create some PRs for them.

Just had a quick look at your platform branch

Yep, being able to split the class implementation up like that and conditionally include the parts via CMake is quite nice.

Interesting point regarding the shutdown.

My thoughts came down to whether the live reload of the assets was a critical function or not. I'm happy to leave it as is; one wrinkle about making it not a critical error is communicating it to the user (console message would do) but then also addressing the issue of attempting a re-enable etc. All that is much more complicated than it would strictly need to be :)

msbit commented 3 years ago

I'm not sure about how to handle the file locking conflict under Windows, I had some success implementing retries with a small timeout at https://github.com/stewartmatheson/projectx/blob/691c5d5767a4ff55b491789b7b74dfe486c4abcc/src/SpriteSheet.cpp#L64 but that's running on the main thread, so any blocking code is a Bad Idea ™

msbit commented 3 years ago

^ any additional useless blocking code, obviously some blocking code still needs to happen :)

msbit commented 3 years ago

I think that about wraps up the asset watcher! 💯