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

Adding watch option using dmon #1308

Merged
merged 37 commits into from
Mar 25, 2024
Merged

Conversation

mwestphal
Copy link
Contributor

@mwestphal mwestphal commented Mar 10, 2024

This add support for an "--watch" cli option where the current file is reloaded automatically if changed on disk.
It relies on https://github.com/septag/dmon for detecting the changes.

In order to support this properly, F3DStarter is reworked with a proper event loops that currently supports:

  • Reloading the current file
  • Rendering

This also adds a test for it on linux/apple/windows

Script used:

while :
do
  sleep 1
  cp cowlow.vtp a.vtp
  sleep 1
  cp cow.vtp a.vtp
done
Peek.2024-03-17.12-09.mp4

Fix: #86

Copy link

You are modifying libf3d public API! ⚠️Please update bindings accordingly⚠️!
You can find them in their respective directories: python, java, webassembly.

@mwestphal mwestphal force-pushed the dmon_auto_reload branch 3 times, most recently from 3222afc to f73f8df Compare March 17, 2024 11:12
application/F3DStarter.cxx Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.48%. Comparing base (eb2d844) to head (e1d8811).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1308      +/-   ##
==========================================
+ Coverage   96.45%   96.48%   +0.03%     
==========================================
  Files         146      146              
  Lines        8765     8793      +28     
==========================================
+ Hits         8454     8484      +30     
+ Misses        311      309       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

application/F3DStarter.cxx Outdated Show resolved Hide resolved
@mwestphal mwestphal force-pushed the dmon_auto_reload branch 2 times, most recently from 9373cca to 096ba11 Compare March 20, 2024 20:27
@mwestphal mwestphal changed the title [Draft] Adding autoreload using dmon Adding auto-reload option using dmon Mar 20, 2024
application/F3DOptionsParser.cxx Outdated Show resolved Hide resolved
application/F3DStarter.cxx Outdated Show resolved Hide resolved
application/F3DStarter.h Outdated Show resolved Hide resolved
application/F3DStarter.h Outdated Show resolved Hide resolved
@snoyer
Copy link
Contributor

snoyer commented Mar 24, 2024

Looks like you already have the implementation details covered so I'm left with nitpicking naming:

  • ForceRender() just performs the render it doesn't take any extra steps to "force" it -> Render()
  • Render() doens't actually perform the render, it only asks for the render to be done (or declares the current render outdated depending on how you want to look at it) -> RequestRender(), InvalidateRender(), ...
  • SoftRender is a flag but is named like an action -> RenderRequested, RenderNeeded, RenderFlag, ... (same for ReloadFile -> ReloadRequested, ...)

tldr:

void Render() { SoftRender = true; } // do the thing = don't actually  o_O
if (SoftRender) ForceRender(); // if soft then forceful  o_O
void RequestRender() { RenderRequested = true; } // request the thing = mark it as requested  ^_^
if (RenderRequested) Render(); // do the thing if asked  ^_^

@mwestphal
Copy link
Contributor Author

As always, the easiest to find out how to do the right thing is just to do the wrong thing and wait for people to fix it for you, thanks @snoyer :)

@mwestphal mwestphal changed the title Adding auto-reload option using dmon Adding watch option using dmon Mar 24, 2024
.github/workflows/ci.yml Outdated Show resolved Hide resolved
application/testing/test_watch.sh Show resolved Hide resolved
Copy link
Contributor

@Meakk Meakk left a comment

Choose a reason for hiding this comment

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

small typo to fix and good to go!

application/testing/CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Michael MIGLIORE <[email protected]>
@mwestphal mwestphal merged commit ec40b6e into f3d-app:master Mar 25, 2024
41 checks passed
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.

Option to automatically reload the active file
3 participants