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

Remove type check when iterating upstream #2055

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

Conversation

adamgerhant
Copy link
Collaborator

I changed the upstream traversal for the tools to only iterate over the same type so that modifications to the output of a Boolean were applied to the vector data after the operation, not an upstream path node. This was a hacky solution, and we need better support for selection which path node to modify. This can be done with dots in the layer panel. More information in comment above existing_node_id

Copy link

Found Clippy warnings

Clippy Warnings/Errors

warning: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
   --> node-graph/gcore/src/raster/adjustments.rs:813:3
    |
813 | /         match self {
814 | |             Some(ref mut v) => *v = map_fn(v),
815 | |             None => (),
816 | |         }
    | |_________^ help: try: `if let Some(ref mut v) = self { *v = map_fn(v) }`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match
    = note: `-W clippy::single-match` implied by `-W clippy::all`
    = help: to override `-W clippy::all` add `#[allow(clippy::single_match)]`

warning: `graphene-core` (lib) generated 1 warning (run `cargo clippy --fix --lib -p graphene-core` to apply 1 suggestion)
warning: large size difference between variants
   --> node-graph/graph-craft/src/document/value.rs:28:3
    |
28  | /         pub enum TaggedValue {
29  | |             None,
30  | |             $( $(#[$meta] ) *$identifier( $ty ), )*
    | |                              ------------------ the largest variant contains at least 648 bytes
31  | |             RenderOutput(RenderOutput),
    | |             -------------------------- the second-largest variant contains at least 216 bytes
...   |
34  | |             EditorApi(Arc<WasmEditorApi>)
35  | |         }
    | |_________^ the entire enum is at least 0 bytes
...
116 | / tagged_value! {
117 | |     String(String),
118 | |     U32(u32),
119 | |     U64(u64),
...   |
181 | |     FontCache(Arc<graphene_core::text::FontCache>),
182 | | }
    | |_- in this macro invocation
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
    = note: `-W clippy::large-enum-variant` implied by `-W clippy::all`
    = help: to override `-W clippy::all` add `#[allow(clippy::large_enum_variant)]`
    = note: this warning originates in the macro `tagged_value` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider boxing the large fields to reduce the total size of the enum
    |
138 |     VectorData(Box<graphene_core::vector::VectorData>),
    |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warning: `graph-craft` (lib) generated 1 warning
warning: `graphene-core` (lib) generated 1 warning (1 duplicate)
warning: `graph-craft` (lib) generated 1 warning (1 duplicate)
warning: `graph-craft` (lib test) generated 1 warning (1 duplicate)
warning: use of deprecated type alias `std::panic::PanicInfo`: use `PanicHookInfo` instead
  --> website/other/bezier-rs-demos/wasm/src/lib.rs:22:36
   |
22 |         fn panic_hook(info: &std::panic::PanicInfo<'_>) {
   |                                          ^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: `bezier-rs-wasm` (lib) generated 1 warning
warning: `bezier-rs-wasm` (lib test) generated 1 warning (1 duplicate)
warning: `graphene-core` (lib test) generated 1 warning (1 duplicate)
    Finished `dev` profile [optimized + debuginfo] target(s) in 4m 58s

Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

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

The code looks good.

Automatically inserting the flatten elements before the path node foregrounds an issue with the ID collisions. If you have a group with two shapes with the same point ids (e.g. two rectangles) and flatten them, the ids for the second rectangle differ each time, meaning that it is not possible to drag them around.

https://github.com/GraphiteEditor/Graphite/blob/c3a3c4c907419e2ebb9a382546a1d7b66f57cdc0/node-graph/gcore/src/vector/vector_data/attributes.rs#L762C78-L763C1

@adamgerhant
Copy link
Collaborator Author

adamgerhant commented Oct 22, 2024

Yes, that is why I created the generate_from_hash, which generates stable yet unique ids based on the NodeId of the merge node that the vector data flows through. It is implemented for the flatten vector elements in #2049, here where I needed the functionality to add the joining of vector data with the pen tool (this should be split into a separate PR).

It is also currently used by the repeat and copy to point nodes so that each new vector data has stable and unique ids.

This is still a work around to allowing the path tool to operate directly on vector data. A more robust method is necessary for the path node to input and output a GraphicGroup. I suppose each vector data will need a unique ID that the path node can reference, since the merge node id will change when it is copy and pasted.

@adamgerhant adamgerhant marked this pull request as draft October 22, 2024 03:29
@adamgerhant
Copy link
Collaborator Author

@Keavon Noted a crash on this branch, but I have not been able to reproduce it

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