-
Notifications
You must be signed in to change notification settings - Fork 50
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
feature: expose ICC profile #47
feature: expose ICC profile #47
Conversation
packages/psd/src/classes/Psd.ts
Outdated
@@ -49,9 +50,14 @@ export class Psd extends Synthesizable implements NodeBase<never, NodeChild> { | |||
switch (resource.id) { | |||
case ResourceType.GridAndGuides: | |||
this.guides = resource.resource.guides; | |||
break; | |||
continue; |
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.
Is it okay for change break
statement into continue
statement?
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.
Very good question. When I first looked at the code I assumed the original intention was to "map over all resources and assign them to proper slots". In this spirit break
seems like a weird choice, since it'd stop the iteration early (before reading all resources).
But it's also possible that intention was different and/or I misunderstand how switch cases work in TS.
Tests pass in either case and there's not really much history in Git to see what the intention was. What do you think would be best?
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.
Ahh, I see - I misunderstood JS in this case. The break
would refer to the switch statement, whilst continue
applies to for-loop. Ugh. Reverting :)
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch Break here refers to enclosing switch statement, not for-loop.
case ResourceType.Slices: | ||
this.slices = loadSlicesFromResourceBlock(resource); | ||
continue; | ||
break; |
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.
👍
@pastelmind any chance on getting this reviewed? The good news is that it's enough to implement the conversion: I created simple demo. |
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.
Sorry for late. LGTM!
…file feature: expose ICC profile
@scoiatael sorry to bother you. could you provide your simple demo source code , I want to restore color with icc profile srgb2.1, but i don't know how to do it, thank you very much ~ |
@windyrain there's actually a library we are currently working on inside OpenDesign with @alexspevak: https://www.npmjs.com/package/@opendesign/image-icc-profile-converter It's currently very alpha, but it should spare you the pain of building C libraries via emscripten. If you encounter any problems or have more question, feel free to open an issue on https://github.com/opendesigndev/psd-ts describing details :) |
fixes #46
I went for the dumbest implementation - just expose the underlying
Uint8Array
. As far as I can tell this produces a correct profile (can be opened and used with Little-CMS CLI tools).I'll work next week on integrating emscripten Little-CMS build into our internal parser, so will come back with more feedback :)
I'm a little worried about our fixtures growing - this might be a potential repo bloat in the future, unless we start using something like Git LFS for them?