Ambient icon indicating copy to clipboard operation
Ambient copied to clipboard

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

Open fabiopolimeni opened this issue 1 year ago • 11 comments

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.

fabiopolimeni avatar Nov 20 '23 11:11 fabiopolimeni

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.

ten3roberts avatar Nov 20 '23 11:11 ten3roberts

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?

fabiopolimeni avatar Nov 20 '23 12:11 fabiopolimeni

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.

ten3roberts avatar Nov 20 '23 12:11 ten3roberts

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

Pombal avatar Nov 20 '23 14:11 Pombal

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.

philpax avatar Nov 20 '23 14:11 philpax

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.

fabiopolimeni avatar Nov 20 '23 14:11 fabiopolimeni

@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?

Pombal avatar Nov 20 '23 14:11 Pombal

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?

philpax avatar Nov 20 '23 14:11 philpax

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?

fabiopolimeni avatar Nov 22 '23 11:11 fabiopolimeni

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?

philpax avatar Nov 22 '23 13:11 philpax

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

Of course

fabiopolimeni avatar Nov 22 '23 15:11 fabiopolimeni