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

live-preview: Reload when images change on disc #6521

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hunger
Copy link
Member

@hunger hunger commented Oct 11, 2024

These are two patches:

  1. Store the absolute image path to the Expression::ImageReference
  2. Use that data in the live-preview

I am a bit uncertain whether 1. is actually needed: The "normal" images I expect to see in the Live Preview have a resource_ref of Absolute(String), which contains the absolute path to the image already. I just wanted to make sure this will continue to work, even if we start to embed images (or decide to add some other way to access image data).

Something else I am not too happy about is that I am back to watching **/*. I suspect that VSCode watches everything anyway and the pattern I pass to VSCode is just filtering what gets sent to my LS (which reduces the amount of data to serialize/sent/deserialize by a bit).

Store the path to the image resource in 
`Expression::ImageReference`, so that we have the
information available later independet of how we embed
the images.
Reload the preview whenever the image resources change
on disk.
@@ -912,6 +913,27 @@ fn start_parsing() {
send_status("Loading Preview…", Health::Ok);
}

fn extract_resources(handle: &ComponentInstance) -> HashSet<Url> {
let mut result: HashSet<Url> = Default::default();
Copy link
Member

@tronical tronical Oct 11, 2024

Choose a reason for hiding this comment

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

If you have a ComponentInstance, then I think that perhaps it would be better to "compile" with compiler_config.embed_resources being set to EmbedAllResources, re-use the existing logic to collect resources (such as imported TrueType fonts), and then use the Document's embedded_file_resources field.

That has the absolute paths you're looking for and then you don't need to extend the image reference expression in the compiler.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the ComponentInstance I use for the preview.

I need to test whether I can do this while keeping the preview functional in WASM. IIRC we use web technologies to display (some of?) the images, so not sure how well that will work with embedded images.

Maybe we could put an Option into Document::embedded_file_resources (and maybe rename to resources?) and put the external images as a None into that? That would get rid of the field in resource_ref.

Copy link
Member

Choose a reason for hiding this comment

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

The use of web technologies doesn't happen in the compiler, that's at run-time. The compiler only reads the actual resource files (ttf, png, etc.) at the code generator stage, which isn't run here. (That's not entirely true, it does read earlier if EmbedResourcesKind::EmbedTextures, but we shouldn't use that here).

Copy link
Member

@tronical tronical Oct 11, 2024

Choose a reason for hiding this comment

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

Differently put:

pub struct Document {
    pub node: Option<syntax_nodes::Document>,
    pub inner_components: Vec<Rc<Component>>,
    pub inner_types: Vec<Type>,
    pub local_registry: TypeRegister,
    /// A list of paths to .ttf/.ttc files that are supposed to be registered on
    /// startup for custom font use.
    pub custom_fonts: Vec<(String, crate::parser::SyntaxToken)>,
    pub exports: Exports,

    /// Map of resources that should be embedded in the generated code, indexed by their absolute path on
    /// disk on the build system
    pub embedded_file_resources:
        RefCell<HashMap<String, crate::embedded_resources::EmbeddedResources>>,

    /// The list of used extra types used recursively.
    pub used_types: RefCell<UsedSubTypes>,
}

This is literally your vector of absolute file paths:

document.embedded_file_resources.borrow().keys().collect::<Vec<_>>;

provided you use EmbedAllResources.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeap,

document.embedded_file_resources.borrow().keys().collect::<Vec<_>>;

is the list of absolute file paths I need.

I am just not sure I can use that:

The embed_images pass fills in the Document's embedded_file_resources. It does so when updating Expression::ImageReference::AbsolutePath(...) (which get populated in the resolving pass) with E::IR::Embedded* ones.

That worries me, considering that the interpreter hits todo!() for any E::IR::Embedded* it encounters. That seems to imply to me that it is not safe to ask the compiler to embed when using the interpreter.

We could of course register all resources independent of whether they are actually getting embedded or not. That would entail having a "this is not embedded"-marker for Document::embedded_file_resources (and renaming that to Document::resources or something;-).

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tried it for good measure: The preview panics by hitting an todo! when I embed resources.

Copy link
Member Author

@hunger hunger Oct 11, 2024

Choose a reason for hiding this comment

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

#6525 adds a new resource embedding mode ListAllResources that fills in Document::embedded_file_resources with None when it finds a resource but does not embed it.

Just to get an idea what such a change would look like. If that looks good, then I can move the (small) rest of this PR on top of that and get rid of the custom resource finding code.

Copy link
Member

Choose a reason for hiding this comment

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

I was just trying to place the resource map into the interpreter after compilation, so that we can resolve the resource_id in ImageReference, but that's a fair bit more work that your approach. That seems like a reasonable compromise to me. I'll comment over there :)

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.

2 participants