Skip to content

Reimplement Brightness/Contrast node using the Curves node to reduce code#1434

Merged
0HyperCube merged 6 commits into
GraphiteEditor:masterfrom
jafriyie1:cleanup-brightness-node
Nov 2, 2023
Merged

Reimplement Brightness/Contrast node using the Curves node to reduce code#1434
0HyperCube merged 6 commits into
GraphiteEditor:masterfrom
jafriyie1:cleanup-brightness-node

Conversation

@jafriyie1

Copy link
Copy Markdown
Contributor

Closes #1387

@Keavon Keavon changed the title Cleanup: Brightness/Contrast node implementation to reuse Curves node Reimplement Brightness/Contrast node using the Curves node to reduce code Oct 17, 2023
@Keavon Keavon marked this pull request as draft October 17, 2023 00:43
@jafriyie1

Copy link
Copy Markdown
Contributor Author

@Keavon I ran cargo test and the tests passed. I think this is good to go whenever you are ready!

@0HyperCube 0HyperCube marked this pull request as ready for review October 30, 2023 14:22

@0HyperCube 0HyperCube left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@jafriyie1

Copy link
Copy Markdown
Contributor Author

Great thanks @0HyperCube for the review! It looks like the pull request step is failing in the Github Actions related to the Build Graphite Web step. Is there something else that I need to here? Thanks!

For context here is part of the error message:

2023-10-30 14:26:43.974993965 +00:00:00 [WARN] failed to request license information from clearly defined: error sending request for url (https://api.clearlydefined.io/definitions): operation timed out
2023-10-30 14:26:43.975018431 +00:00:00 [WARN] failed to request license information from clearly defined: error sending request for url (https://api.clearlydefined.io/definitions): operation timed out

Could not run `cargo about`, which is required to generate license information.
To install cargo-about on your system, you can run `cargo install cargo-about`.
License information is required on production builds. Aborting.

> print-building-help
> echo 'Graphite project failed to build. Did you remember to `npm install` the dependencies in `/frontend`?'

Graphite project failed to build. Did you remember to `npm install` the dependencies in `/frontend`?```

@Keavon

Keavon commented Nov 2, 2023

Copy link
Copy Markdown
Member

As long as you've run the tests which your code influences, you're good. There are other tests in our CI pipeline that are currently failing after we decided to merge a giant refactor before we could fix its tests (not usually desired, but it was needed in this case).

@0HyperCube 0HyperCube merged commit c823016 into GraphiteEditor:master Nov 2, 2023
@Keavon

Keavon commented Jun 13, 2026

Copy link
Copy Markdown
Member

Hi @jafriyie1, thanks again for this code contribution to the project! We're still hoping you will respond to the request to relicense this code. Please see #4208 ASAP, thank you!

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.

Change Brightness/Contrast node implementation to reuse Curves node

3 participants