-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make round stroke caps for tool strokes #3962
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
266ac44
19975f7
e98a8db
36233b4
bed400f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,14 @@ use std::path::PathBuf; | |
| pub fn main() -> Result<(), Box<dyn std::error::Error>> { | ||
| env_logger::builder().filter_level(log::LevelFilter::Debug).init(); | ||
|
|
||
| // This crate is often built for wasm32 via wasm-pack, which sets `CARGO_BUILD_TARGET`. | ||
| // cargo-gpu then spawns nested `cargo` to build `rustc_codegen_spirv` for the host; if that | ||
| // variable is inherited, the nested build targets wasm32 and fails with "cannot produce dylib". | ||
| // SAFETY: Build script is single-threaded. | ||
| unsafe { | ||
|
||
| std::env::remove_var("CARGO_BUILD_TARGET"); | ||
| } | ||
|
|
||
| // Skip building the shaders if they are provided externally | ||
| println!("cargo:rerun-if-env-changed=RASTER_NODES_SHADER_PATH"); | ||
| if !std::env::var("RASTER_NODES_SHADER_PATH").unwrap_or_default().is_empty() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Custom agent: PR title enforcement
PR title must start with an approved imperative verb from the Leading Verb Vocabulary; "Use" is not allowed. Rename the title to start with an approved verb such as "Make" or "Fix" (for example, "Make round stroke caps for tool strokes and fix viewport tile cache...").
Prompt for AI agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Veercodeprog Nice work on the PR title update—it now correctly uses an imperative verb.
I have a few technical suggestions to ensure the tile cache invalidation is as robust as possible and to clean up the new build logic:
1. Map missing flags in
create_contextIn
node-graph/nodes/gstd/src/render_node.rs, you are correctly passing thedocument_network_hashfromRenderConfigtoRenderParams. However, there are several other flags inRenderConfig(likefor_exportandhide_artboards) that are currently being ignored and falling back toDefault.If these aren't mapped, the cache might not distinguish between a viewport render and an export render of the same network, which could lead to stale pixels if those modes have different rendering logic (e.g., background transparency).
2. Environment modification in
build.rsThe use of
unsafeforstd::env::remove_varinnode-graph/nodes/raster/shaders/build.rsis appropriate here. Since Rust 1.81, environment mutation is markedunsafebecause it's not thread-safe. Your justification—that the build script is single-threaded and needs to clearCARGO_BUILD_TARGETto preventwasm-packfrom breaking nested host builds—is correct and follows standard practice for this specificrust-gpuissue.3. Verify
network_hashcoverageThe technical note mentions that changing stroke properties invalidates the cache. This works because the
document_network_hashincludes all node input values. Just double-check that the tools you've updated (freehand, spline, etc.) correctly trigger a network state change when their properties are tweaked via the UI, as the cache now relies entirely on that hash for its identity.For more details on how we handle rendering state and caching, you can check the Graphite Wiki.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Veercodeprog The changes look solid and address the stale viewport tile issue effectively. I suggested the title change earlier because the project follows a convention requiring imperative verbs (like "Make" or "Fix") to keep the history clear and actionable.
Here are a few technical observations and suggestions based on your implementation:
1. Cache Invalidation Logic
Including the
document_network_hashin theCacheKeyandRenderParamsis a robust way to ensure that any change to the node graph topology or inputs forces a re-render.2. Default Stroke Cap
Changing the default to
StrokeCap::Roundinapply_strokeis a good UX improvement for tools like the Freehand and Pen tools, as it makes paths feel "finished" immediately.3. WASM Build Fix in
build.rsThe addition of the
unsafeblock to removeCARGO_BUILD_TARGETin the shader build script is a smart fix.wasm-pack,CARGO_BUILD_TARGETis set towasm32-unknown-unknown. This environment variable is inherited by the nestedcargocalls made bycargo-gputo build the SPIR-V compiler for the host machine. Without removing it, the host build fails because it tries to compile the compiler itself for WASM.unsafewith the "single-threaded" justification matches the existing patterns in the codebase for environment manipulation in build scripts.4. Code Quality
Hashimplementation forRenderParamscorrectly includes the newdocument_network_hashfield, maintaining the invariant that equal parameters produce equal hashes.#[serde(default)]to thedocument_network_hashinRenderConfigis a good practice for maintaining compatibility across different parts of the system.One minor suggestion:
In
editor/src/node_graph_executor.rs, you calldocument.network_interface.network_hash()three times across different rendering paths (viewport, eyedropper, export). While this is fine, if these branches were ever merged or shared logic, you might consider fetching it once at the start of the execution block to ensure consistency within a single render pass, though in its current form, it's perfectly safe.Overall, the PR is in great shape.
Learn more about Graphite's rendering architecture
Cubic Settings