Simple gpu airbrush - #4469
Conversation
There was a problem hiding this comment.
11 issues found across 29 files
Confidence score: 1/5
node-graph/nodes/brush/src/airbrush/pipeline.rscan reject every scatter pass becauseScatterUniformsis bound at 24 bytes instead of the required 32, and some adapters cannot createScatterwith blendedR16Floatattachments — pad the uniform and use or validate a blendable density format.editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rspanics whenbrush_strokesis missing from the registry, turning invalid brush operations into an editor crash — replace theexpectwith a logged early return.editor/src/messages/portfolio/document/document_message_handler.rsremoves undo/redo history whenever.gddsaving is enabled, so reopening a saved document loses its editing history — preserve history for normal document saves.node-graph/nodes/brush/src/airbrush/mod.rscan render no strokes for boxedGraphic::Graphicinput and can panic on mismatched stroke channel lengths; traverse the boxed form and validate strokes before indexing their samples.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/nodes/brush/src/airbrush/pipeline.rs">
<violation number="1" location="node-graph/nodes/brush/src/airbrush/pipeline.rs:14">
P1: On adapters where `R16Float` is not blendable, creating `Scatter` fails because both density attachments request blending. Use a guaranteed blendable density format or check the adapter’s format capabilities before enabling this pipeline.</violation>
<violation number="2" location="node-graph/nodes/brush/src/airbrush/pipeline.rs:24">
P1: The scatter draw binds a 24-byte buffer for a uniform block requiring 32-byte layout size, so wgpu validation can reject every scatter pass. Pad `ScatterUniforms` to 32 bytes and update its initializer.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:884">
P1: When the brush node registry cannot resolve `brush_strokes`, this handler panics at `.expect(...)` instead of rejecting the brush operation. Handle the missing definition with a logged early return so an invalid registry state cannot crash the editor.
(Based on your team's feedback about avoiding panics in application code.)</violation>
</file>
<file name="editor/src/messages/preferences/preferences_message_handler.rs">
<violation number="1" location="editor/src/messages/preferences/preferences_message_handler.rs:96">
P2: When loading preferences disables the brush tool while Brush is active, this refresh hides Brush in the shelf but keeps Brush active. Mirror the `PreferencesMessage::BrushTool` deactivation check in the load path before refreshing the shelf.</violation>
</file>
<file name="editor/src/messages/portfolio/document/node_graph/node_properties.rs">
<violation number="1" location="editor/src/messages/portfolio/document/node_graph/node_properties.rs:289">
P2: When the brush-strokes node is selected, its `List<Stroke>` input now falls through to the unsupported-widget placeholder because this change removes the only brush-stroke summary widget. Restore an equivalent `Stroke`-list widget, or add the new list shape to the supported property dispatch.</violation>
</file>
<file name="node-graph/nodes/brush/src/airbrush/mod.rs">
<violation number="1" location="node-graph/nodes/brush/src/airbrush/mod.rs:37">
P2: When a stroke has a channel length different from its position count, the airbrush renderer panics while indexing `Channel::Samples`. Validate `stroke.is_valid()` before constructing `StyledStroke`, or otherwise skip invalid strokes.</violation>
<violation number="2" location="node-graph/nodes/brush/src/airbrush/mod.rs:44">
P2: When the input contains the boxed `Graphic::Graphic` form, `airbrush` drops the group and renders no contained strokes. Traverse `Graphic::Graphic(item)` through a one-item list before handling the other graphic variants.</violation>
</file>
<file name="node-graph/nodes/brush/src/airbrush/kernel.rs">
<violation number="1" location="node-graph/nodes/brush/src/airbrush/kernel.rs:61">
P2: If a cache bake or another operation panics while this guard is held, every later airbrush render panics at `unwrap()` because the mutex is poisoned. Handle the poisoned lock explicitly and bypass caching or recover the guard so a cache failure does not crash rendering.
(Based on your team's feedback about avoiding panics in application code.)</violation>
</file>
<file name="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:1070">
P2: When `.gdd` saving is enabled, this drops the document's undo/redo history on every save. Reopening then bootstraps a flat registry instead of restoring the saved history; keep history enabled for normal document saves.</violation>
</file>
<file name="node-graph/nodes/brush/src/airbrush/stroke.rs">
<violation number="1" location="node-graph/nodes/brush/src/airbrush/stroke.rs:115">
P2: When pressure changes at an unchanged position after the first kept dab, this branch drops the new pressure-dependent dab. Emit a dab when the current sigma differs from `kept_last.sigma` instead of suppressing it unconditionally.</violation>
</file>
<file name="node-graph/nodes/brush/src/airbrush/region.rs">
<violation number="1" location="node-graph/nodes/brush/src/airbrush/region.rs:1">
P2: Custom agent: **PR title enforcement**
The PR title does not meet the required format: `Simple` is not an imperative leading verb, the title has only three words, and `gpu` should be `GPU`. Rename it to `Add a GPU airbrush`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| use raster_types::Texture; | ||
| use wgpu_executor::{AsyncWgpuPipeline, Buffer, WgpuExecutor}; | ||
|
|
||
| pub(super) const DENSITY_FORMAT: wgpu::TextureFormat = wgpu::TextureFormat::R16Float; |
There was a problem hiding this comment.
P1: On adapters where R16Float is not blendable, creating Scatter fails because both density attachments request blending. Use a guaranteed blendable density format or check the adapter’s format capabilities before enabling this pipeline.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/brush/src/airbrush/pipeline.rs, line 14:
<comment>On adapters where `R16Float` is not blendable, creating `Scatter` fails because both density attachments request blending. Use a guaranteed blendable density format or check the adapter’s format capabilities before enabling this pipeline.</comment>
<file context>
@@ -0,0 +1,543 @@
+use raster_types::Texture;
+use wgpu_executor::{AsyncWgpuPipeline, Buffer, WgpuExecutor};
+
+pub(super) const DENSITY_FORMAT: wgpu::TextureFormat = wgpu::TextureFormat::R16Float;
+pub(super) const COMPOSITE_FORMAT: wgpu::TextureFormat = wgpu::TextureFormat::Rgba16Float;
+
</file context>
| pub(super) const DENSITY_FORMAT: wgpu::TextureFormat = wgpu::TextureFormat::R16Float; | |
| pub(super) const DENSITY_FORMAT: wgpu::TextureFormat = wgpu::TextureFormat::Rgba16Float; |
| kernel_scale: f32, | ||
| kernel_exponent: f32, | ||
| kernel_section_scale: f32, | ||
| _pad: f32, |
There was a problem hiding this comment.
P1: The scatter draw binds a 24-byte buffer for a uniform block requiring 32-byte layout size, so wgpu validation can reject every scatter pass. Pad ScatterUniforms to 32 bytes and update its initializer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/brush/src/airbrush/pipeline.rs, line 24:
<comment>The scatter draw binds a 24-byte buffer for a uniform block requiring 32-byte layout size, so wgpu validation can reject every scatter pass. Pad `ScatterUniforms` to 32 bytes and update its initializer.</comment>
<file context>
@@ -0,0 +1,543 @@
+ kernel_scale: f32,
+ kernel_exponent: f32,
+ kernel_section_scale: f32,
+ _pad: f32,
+}
+
</file context>
| let strokes_node = resolve_proto_node_type(graphene_std::brush::brush_strokes::IDENTIFIER) | ||
| .expect("Brush strokes node does not exist") |
There was a problem hiding this comment.
P1: When the brush node registry cannot resolve brush_strokes, this handler panics at .expect(...) instead of rejecting the brush operation. Handle the missing definition with a logged early return so an invalid registry state cannot crash the editor.
(Based on your team's feedback about avoiding panics in application code.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, line 884:
<comment>When the brush node registry cannot resolve `brush_strokes`, this handler panics at `.expect(...)` instead of rejecting the brush operation. Handle the missing definition with a logged early return so an invalid registry state cannot crash the editor.
(Based on your team's feedback about avoiding panics in application code.) </comment>
<file context>
@@ -854,6 +880,20 @@ fn import_usvg_node_inner(
}
+fn insert_brush_strokes_chain(network_interface: &mut NodeNetworkInterface, layer: LayerNodeIdentifier, strokes_node_id: NodeId, color: Color, diameter: f64, hardness: f64, flow: f64) {
+ let strokes_node = resolve_proto_node_type(graphene_std::brush::brush_strokes::IDENTIFIER)
+ .expect("Brush strokes node does not exist")
+ .node_template_input_override([
</file context>
| let strokes_node = resolve_proto_node_type(graphene_std::brush::brush_strokes::IDENTIFIER) | |
| .expect("Brush strokes node does not exist") | |
| let Some(strokes_node_definition) = resolve_proto_node_type(graphene_std::brush::brush_strokes::IDENTIFIER) else { | |
| log::error!("Brush strokes node does not exist"); | |
| return; | |
| }; | |
| let strokes_node = strokes_node_definition |
| zoom_with_scroll: self.zoom_with_scroll, | ||
| }); | ||
| responses.add(FrontendMessage::UpdateUIScale { scale: self.ui_scale }); | ||
| responses.add(ToolMessage::RefreshToolShelf); |
There was a problem hiding this comment.
P2: When loading preferences disables the brush tool while Brush is active, this refresh hides Brush in the shelf but keeps Brush active. Mirror the PreferencesMessage::BrushTool deactivation check in the load path before refreshing the shelf.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/preferences/preferences_message_handler.rs, line 96:
<comment>When loading preferences disables the brush tool while Brush is active, this refresh hides Brush in the shelf but keeps Brush active. Mirror the `PreferencesMessage::BrushTool` deactivation check in the load path before refreshing the shelf.</comment>
<file context>
@@ -93,6 +93,7 @@ impl MessageHandler<PreferencesMessage, PreferencesMessageContext<'_>> for Prefe
zoom_with_scroll: self.zoom_with_scroll,
});
responses.add(FrontendMessage::UpdateUIScale { scale: self.ui_scale });
+ responses.add(ToolMessage::RefreshToolShelf);
}
PreferencesMessage::ResetToDefaults => {
</file context>
| responses.add(ToolMessage::RefreshToolShelf); | |
| if !self.brush_tool && tool_message_handler.tool_state.tool_data.active_tool_type == ToolType::Brush { | |
| responses.add(ToolMessage::ActivateToolSelect); | |
| } | |
| responses.add(ToolMessage::RefreshToolShelf); |
| Some(x) if id_is::<DAffine2>(x) => transform_widget(default_info, &mut extra_widgets), | ||
| Some(x) if id_is::<Color>(x) => color_widget(default_info, ColorInput::default().allow_none(false)), | ||
| Some(x) if id_is::<Gradient>(x) => color_widget(default_info, ColorInput::default().allow_none(false)), | ||
| Some(x) if id_is::<BrushTrace>(x) => brush_strokes_widget(default_info).into(), |
There was a problem hiding this comment.
P2: When the brush-strokes node is selected, its List<Stroke> input now falls through to the unsupported-widget placeholder because this change removes the only brush-stroke summary widget. Restore an equivalent Stroke-list widget, or add the new list shape to the supported property dispatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/node_graph/node_properties.rs, line 289:
<comment>When the brush-strokes node is selected, its `List<Stroke>` input now falls through to the unsupported-widget placeholder because this change removes the only brush-stroke summary widget. Restore an equivalent `Stroke`-list widget, or add the new list shape to the supported property dispatch.</comment>
<file context>
@@ -286,7 +285,6 @@ pub(crate) fn property_from_type(
Some(x) if id_is::<Gradient>(x) => color_widget(default_info, ColorInput::default().allow_none(false)),
- Some(x) if id_is::<BrushTrace>(x) => brush_strokes_widget(default_info).into(),
// ============
// STRUCT TYPES
// ============
@@ -460,33 +458,6 @@ pub fn vector_modification_widget(parameter_widgets_info: ParameterWidgetsInfo)
</file context>
| let sharpest = (EDGE_WIDTH_FACTOR * sigma_texels / (2. * MIN_EDGE_TEXELS)).max(1.); | ||
| let exponent = (SOFTEST * (HARDEST / SOFTEST).powf(stroke.hardness.clamp(0., 1.))).min(sharpest); | ||
| let key = (exponent.ln() * KEY_STEPS_PER_LN).round() as i32; | ||
| let mut entries = self.entries.lock().unwrap(); |
There was a problem hiding this comment.
P2: If a cache bake or another operation panics while this guard is held, every later airbrush render panics at unwrap() because the mutex is poisoned. Handle the poisoned lock explicitly and bypass caching or recover the guard so a cache failure does not crash rendering.
(Based on your team's feedback about avoiding panics in application code.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/brush/src/airbrush/kernel.rs, line 61:
<comment>If a cache bake or another operation panics while this guard is held, every later airbrush render panics at `unwrap()` because the mutex is poisoned. Handle the poisoned lock explicitly and bypass caching or recover the guard so a cache failure does not crash rendering.
(Based on your team's feedback about avoiding panics in application code.) </comment>
<file context>
@@ -0,0 +1,163 @@
+ let sharpest = (EDGE_WIDTH_FACTOR * sigma_texels / (2. * MIN_EDGE_TEXELS)).max(1.);
+ let exponent = (SOFTEST * (HARDEST / SOFTEST).powf(stroke.hardness.clamp(0., 1.))).min(sharpest);
+ let key = (exponent.ln() * KEY_STEPS_PER_LN).round() as i32;
+ let mut entries = self.entries.lock().unwrap();
+ if let Some(index) = entries.iter().position(|(cached, _)| *cached == key) {
+ if let Some(texture) = entries[index].1.texture.upgrade() {
</file context>
| let mut entries = self.entries.lock().unwrap(); | |
| let Ok(mut entries) = self.entries.lock() else { | |
| return bake(executor, (key as f64 / KEY_STEPS_PER_LN).exp()); | |
| }; |
| document_format::ExportFormat::Xz, | ||
| document_format::ExportOptions::default(), | ||
| document_format::ExportOptions { | ||
| include_history: false, |
There was a problem hiding this comment.
P2: When .gdd saving is enabled, this drops the document's undo/redo history on every save. Reopening then bootstraps a flat registry instead of restoring the saved history; keep history enabled for normal document saves.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/document_message_handler.rs, line 1070:
<comment>When `.gdd` saving is enabled, this drops the document's undo/redo history on every save. Reopening then bootstraps a flat registry instead of restoring the saved history; keep history enabled for normal document saves.</comment>
<file context>
@@ -1066,7 +1066,10 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
document_format::ExportFormat::Xz,
- document_format::ExportOptions::default(),
+ document_format::ExportOptions {
+ include_history: false,
+ ..Default::default()
+ },
</file context>
| include_history: false, | |
| include_history: true, |
| let hardness = item.attribute_cloned_or(ATTR_HARDNESS, crate::DEFAULT_HARDNESS / 100.); | ||
| let flow = item.attribute_cloned_or(ATTR_FLOW, crate::DEFAULT_FLOW / 100.); | ||
| match item.into_element() { | ||
| Graphic::StrokeList(list) => strokes.extend(list.into_iter().map(Item::into_element).filter(|stroke| !stroke.is_empty()).map(|stroke| stroke::StyledStroke { |
There was a problem hiding this comment.
P2: When a stroke has a channel length different from its position count, the airbrush renderer panics while indexing Channel::Samples. Validate stroke.is_valid() before constructing StyledStroke, or otherwise skip invalid strokes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/brush/src/airbrush/mod.rs, line 37:
<comment>When a stroke has a channel length different from its position count, the airbrush renderer panics while indexing `Channel::Samples`. Validate `stroke.is_valid()` before constructing `StyledStroke`, or otherwise skip invalid strokes.</comment>
<file context>
@@ -0,0 +1,69 @@
+ let hardness = item.attribute_cloned_or(ATTR_HARDNESS, crate::DEFAULT_HARDNESS / 100.);
+ let flow = item.attribute_cloned_or(ATTR_FLOW, crate::DEFAULT_FLOW / 100.);
+ match item.into_element() {
+ Graphic::StrokeList(list) => strokes.extend(list.into_iter().map(Item::into_element).filter(|stroke| !stroke.is_empty()).map(|stroke| stroke::StyledStroke {
+ color,
+ diameter,
</file context>
| let kept_last = self.kept_last?; | ||
| let dab = dab(&stroke.stroke.sample(stroke.stroke.len() - 1), stroke); | ||
| if dab.position == kept_last.position { | ||
| return (self.kept == 1).then_some((kept_last, kept_last)); |
There was a problem hiding this comment.
P2: When pressure changes at an unchanged position after the first kept dab, this branch drops the new pressure-dependent dab. Emit a dab when the current sigma differs from kept_last.sigma instead of suppressing it unconditionally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/brush/src/airbrush/stroke.rs, line 115:
<comment>When pressure changes at an unchanged position after the first kept dab, this branch drops the new pressure-dependent dab. Emit a dab when the current sigma differs from `kept_last.sigma` instead of suppressing it unconditionally.</comment>
<file context>
@@ -0,0 +1,222 @@
+ let kept_last = self.kept_last?;
+ let dab = dab(&stroke.stroke.sample(stroke.stroke.len() - 1), stroke);
+ if dab.position == kept_last.position {
+ return (self.kept == 1).then_some((kept_last, kept_last));
+ }
+ Some((kept_last, dab))
</file context>
| @@ -0,0 +1,67 @@ | |||
| use core_types::math::bbox::AxisAlignedBbox; | |||
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
The PR title does not meet the required format: Simple is not an imperative leading verb, the title has only three words, and gpu should be GPU. Rename it to Add a GPU airbrush.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/brush/src/airbrush/region.rs:
<comment>The PR title does not meet the required format: `Simple` is not an imperative leading verb, the title has only three words, and `gpu` should be `GPU`. Rename it to `Add a GPU airbrush`.</comment>
<file context>
@@ -0,0 +1,67 @@
+use core_types::math::bbox::AxisAlignedBbox;
+use core_types::transform::Footprint;
+use glam::{DAffine2, DVec2, UVec2};
+
+const MAX_RESOLUTION: u32 = 8192;
+
+const CROP_STEP: u32 = 256;
+
+#[derive(Clone, Copy, PartialEq)]
</file context>
0d9e328 to
11aec5d
Compare
|
!build desktop (Run ID 32505605578) |
|
11aec5d to
abd5f98
Compare
|
|
abd5f98 to
c9e3201
Compare
c9e3201 to
6016bce
Compare
|
!build desktop (Run ID 32536948337) |
|
|
|
6016bce to
e8d0146
Compare
|
!build desktop (Run ID 32567085875) |
|
|
|
No description provided.