AmbientRun / Ambient

The multiplayer game engine
https://ambient.run
Apache License 2.0
3.79k stars 122 forks source link

When only asset filenames change `ambient build` doesn't seem to pick them up #1176

Open fabiopolimeni opened 10 months ago

fabiopolimeni commented 10 months ago

It seems that if I rename an asset file, for instance, a GLB file packed with animations, and then execute an ambient build the project will not be rebuilt, and therefore, my new packaged animations path will not be created. I suspect ambient build does not figure it needs to rebuild the pipelines because no rust source file has changed?

I use VSCode and build through Command+Shift+B on MacOS.

ten3roberts commented 10 months ago

It does not at the moment.

We currently have ambient build --clean-build which will purge the built assets before building. This is also useful to ensure that there are no removed assets that linger in the build folder and keep technically broken links working locally.

fabiopolimeni commented 10 months ago

Yep, that will rebuild the whole project, leading to those unknowns where someone says "It doesn't work" and the other reply "try ambient build --clean-build" and it will magically work XD. Also, I can imagine content creators, who want to quickly iterate through their asset changes, not to want to rebuild the whole project to see their changes apply.

Is there any reason ambient build doesn't watch the asset/ folder, or it is just a matter of adding the functionality to the CLI?

ten3roberts commented 10 months ago

Not sure, I have not personally worked on that part.

I know that there has been a bunch of discussion for how to accurately hash and detect these without leading to cycles, there's been discussion about hashing and baking those into the package itself, to solve this issue when publishing, see: #792.

Pombal commented 10 months ago

@FredrikNoren and @philpax may have more insight on this.

philpax commented 10 months ago

Interesting - the build-skip check checks if any files have a modified date after the last build. My guess is that renaming a file doesn't count as a modification? 🤔

The proper fix is probably #792, as Freja mentioned - we should hash the inputs and use that for change detection.

fabiopolimeni commented 10 months ago

If you are already checking the file modifications, then for adding removing or renaming files, possibly we need to check whether the directory changed as well, not only the files. Unless, that is what we are already doing.

Pombal commented 10 months ago

@fabiopolimeni Was this on Windows or Mac? The Win32 API GetFileTime should be able to get modification timestamps from renaming. I would be surprised if Unix based platforms don't have something similar. What Rust function are we using to detect modifications and what OS system calls does it go on to call?

philpax commented 10 months ago
let last_modified_time = get_files_in_path(&package_path)
    .filter(|p| !p.starts_with(&build_path) && !p.starts_with(&package_individual_build_path))
    .filter_map(|f| f.metadata().ok()?.modified().ok())
    .map(chrono::DateTime::<chrono::Utc>::from)
    .chain(dependency_max_last_build_times.into_iter())
    .max();

let last_modified_before_build = last_build_time
    .zip(last_modified_time)
    .is_some_and(|(build, modified)| modified < build);

https://doc.rust-lang.org/std/fs/struct.Metadata.html#method.modified

EDIT: perhaps we consider both modified and created?

fabiopolimeni commented 10 months ago
let last_modified_time = get_files_in_path(&package_path)
    .filter(|p| !p.starts_with(&build_path) && !p.starts_with(&package_individual_build_path))
    .filter_map(|f| f.metadata().ok()?.modified().ok())
    .map(chrono::DateTime::<chrono::Utc>::from)
    .chain(dependency_max_last_build_times.into_iter())
    .max();

let last_modified_before_build = last_build_time
    .zip(last_modified_time)
    .is_some_and(|(build, modified)| modified < build);

https://doc.rust-lang.org/std/fs/struct.Metadata.html#method.modified

EDIT: perhaps we consider both modified and created?

I would say so, modified and created. Also, I assume this means we are not going through the same code path when building pipeline for the first time? How is it picking the files under assets/ otherwise the first time it sees them?

philpax commented 10 months ago
let last_modified_time = get_files_in_path(&package_path)
    .filter(|p| !p.starts_with(&build_path) && !p.starts_with(&package_individual_build_path))
    .filter_map(|f| f.metadata().ok()?.modified().ok())
    .map(chrono::DateTime::<chrono::Utc>::from)
    .chain(dependency_max_last_build_times.into_iter())
    .max();

let last_modified_before_build = last_build_time
    .zip(last_modified_time)
    .is_some_and(|(build, modified)| modified < build);

https://doc.rust-lang.org/std/fs/struct.Metadata.html#method.modified EDIT: perhaps we consider both modified and created?

I would say so, modified and created. Also, I assume this means we are not going through the same code path when building pipeline for the first time? How is it picking the files under assets/ otherwise the first time it sees them?

That should still work OK - it'll just pick up zero built files (get_files_in_path returns an Iterator), and the rest of the logic will just cancel out. (last_build_time and last_modified_time are Options, so the comparison will only run if they're both present.)

I can add created, but would you like to give it a shot first?

fabiopolimeni commented 10 months ago

I can add created, but would you like to give it a shot first?

Of course