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

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

Open
fabiopolimeni opened this issue Nov 20, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request topic:assets Asset loading, streaming, etc

Comments

@fabiopolimeni
Copy link
Contributor

fabiopolimeni commented Nov 20, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

@fabiopolimeni fabiopolimeni added enhancement New feature or request topic:assets Asset loading, streaming, etc labels Nov 20, 2023
@Pombal
Copy link
Contributor

Pombal commented Nov 20, 2023

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

@philpax
Copy link
Contributor

philpax commented Nov 20, 2023

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
Copy link
Contributor Author

fabiopolimeni commented Nov 20, 2023 via email

@Pombal
Copy link
Contributor

Pombal commented Nov 20, 2023

@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
Copy link
Contributor

philpax commented Nov 20, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

philpax commented Nov 22, 2023

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
Copy link
Contributor Author

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

Of course

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request topic:assets Asset loading, streaming, etc
Projects
None yet
Development

No branches or pull requests

4 participants