Add group layers support #13

tdakkota · 2020-04-05T02:57:54Z

Fixes #10

chinedufn

AWESOME work as usual!

Left some feedback. Note that I only spent 20-30 mins thinking about this so there will be places where I'm completely wrong or missing the mark. Feel free to call those out.

Thanks! Nice work!!

chinedufn · 2020-04-05T11:36:43Z

src/lib.rs

@@ -117,24 +117,19 @@ impl Psd {
 impl Psd {
    /// Get all of the layers in the PSD
    pub fn layers(&self) -> &Vec<PsdLayer> {
-        &self.layer_and_mask_information_section.layers
+        self.layer_and_mask_information_section.group.layers()


What's the idea behind this change?

chinedufn · 2020-04-05T11:45:02Z

src/sections/layer_and_mask_information_section/layer.rs

+}
+
+impl PsdLayerGroup {
+    /// Get all of the layers in the PSD


Isn't this all of the layers in the group, not necessarily the entire PSD?

chinedufn · 2020-04-05T11:47:38Z

src/sections/layer_and_mask_information_section/layer.rs

+pub struct PsdLayerGroup {
+    pub(in crate) layers: Vec<PsdLayer>,
+    /// A map of layer name to index within our layers vector
+    pub(in crate) layer_names: HashMap<String, usize>,


layer_indices or layer_name_indices (I know the name was already there and you just moved it)

chinedufn · 2020-04-05T11:48:15Z

src/sections/layer_and_mask_information_section/layer.rs

+    }
+
+    /// Get a layer by name
+    pub fn layer_by_name(&self, name: &str) -> Result<&PsdLayer, Error> {


This should return Option<&PsdLayer>

Then the code

let layer_idx = self.layer_indices.get(name)?; self.layers.get(layer_idx)

chinedufn · 2020-04-05T11:49:08Z

src/sections/layer_and_mask_information_section/layer.rs

+    /// Get a layer by index.
+    ///
+    /// index 0 is the bottom layer, index 1 is the layer above that, etc
+    pub fn layer_by_idx(&self, idx: usize) -> Result<&PsdLayer, Error> {


Same here should return an option

chinedufn · 2020-04-05T11:58:48Z

src/sections/layer_and_mask_information_section/layer.rs

    // Get one of the PsdLayerChannels of this PsdLayer
    fn get_channel(&self, channel: PsdChannelKind) -> Option<&ChannelBytes> {
        self.channels.get(&channel)
    }
 }

+/// Section divider constants.


No need for these constants. The values can be inlined in the match statement since they're directly next to the enum variants that they map to

chinedufn · 2020-04-05T12:00:39Z

src/sections/layer_and_mask_information_section/layer.rs

+            DIVIDER_OPEN_FOLDER => GroupDivider::OpenFolder,
+            DIVIDER_CLOSED_FOLDER => GroupDivider::CloseFolder,
+            DIVIDER_BOUNDING_SECTION => GroupDivider::BoundingSection,
+            _ => GroupDivider::NotDivider


Similarly here - we have a type GroupDivider that has the ability to not be a divider. More idiomatic to just have an option (a noted below)

chinedufn · 2020-04-05T12:01:16Z

src/sections/layer_and_mask_information_section/layer.rs

@@ -137,6 +237,8 @@ pub struct LayerRecord {
    pub(super) bottom: i32,
    /// The position of the right of the image
    pub(super) right: i32,
+    /// Group divider tag, `GroupDivider::NotDivider` for not divider layers
+    pub(super) divider_type: GroupDivider,


Option<GroupDivider>

chinedufn · 2020-04-05T12:05:03Z

src/sections/layer_and_mask_information_section/mod.rs

-    pub(in crate) layers: Vec<PsdLayer>,
-    /// A map of layer name to index within our layers vector
-    pub(in crate) layer_names: HashMap<String, usize>,
+    pub(crate) group: PsdLayerGroup,


I'm finding this to be confusing

Wouldn't the layer and mask information section have names and groups?

Maybe something like:

pub struct LayerAndMaskInformationSection { layers: Vec<PsdLayer>, layer_indices: HashMap<String, usize>, groups: HashMap<String, Range<usize>> }

chinedufn · 2020-04-05T12:08:22Z

src/sections/layer_and_mask_information_section/layer.rs

-use failure::{Error, Fail};
-use std::collections::HashMap;
+
+/// PsdLayerGroup represents a group of layers


I'm curious about your thoughts on this data structure.

I feel like we should treat the layers as a just bottom to top list of layers .. then have groups just be pointers to some first and last layer. Which is effectively what it is in the spec - just a pointer to the top of one layer and the bottom of another.

Feels more lightweight to be vs. needing to have layers and sub-layers and sub-sub-layers etc.

tdakkota · 2020-04-06T02:41:56Z

So, I've got a question.
What should we do if PSD file/Group contains two layers with the same name?
Which layer we should return?

chinedufn · 2020-04-06T10:08:41Z

Hmm..

Maybe we should be returning Vec<PsdLayer> if its possible for two layers to have the same name?

I'm curious what you suggest here?

chinedufn

A M A Z I N G work - I don't know how you're so fast haha.

Left lots of minor notes. Structurally this looks awesome to me - just some small bits of feedback scattered around.

Love the direction that you went and I learned a couple things!

chinedufn · 2020-04-07T10:51:56Z

src/sections/layer_and_mask_information_section/container.rs

+}
+
+impl<T> KeyIDContainer<T> {
+    /// Creates a new `KeyIDContainer`


A lot of these doc comments don't give any new information. Would replace the ones that are just repeating the method name with an #[allow(missing_docs)] annotation

chinedufn · 2020-04-07T10:52:11Z

src/lib.rs

+
+    /// Get all of the groups in the PSD
+    pub fn groups(&self) -> &Vec<PsdGroup> {
+        &self.layer_and_mask_information_section.groups.items()


Hmmm items seems like an unexpected word - but I'll keep reading the PR to figure out what this means

chinedufn · 2020-04-07T10:53:17Z

src/sections/layer_and_mask_information_section/container.rs

+use std::ops::{Index, Range};
+
+#[derive(Debug)]
+pub(crate) struct KeyIDContainer<T> {


Would doc comment this with the purpose / use case of the abstraction

Also - idea for a name - NamedItems<T>?

chinedufn · 2020-04-07T10:55:40Z

src/sections/layer_and_mask_information_section/container.rs

+    }
+}
+
+impl<T> Index<usize> for KeyIDContainer<T> {


chinedufn · 2020-04-07T10:55:45Z

src/sections/layer_and_mask_information_section/container.rs

+    }
+}
+
+impl<T> Index<&str> for KeyIDContainer<T> {


chinedufn · 2020-04-07T11:14:22Z

src/sections/layer_and_mask_information_section/mod.rs

-
-        let mut parse_group_now = false;
+        let mut range_stack = vec![];
+        let mut nested_level = 0;


Do we need nested_level ? Isn't it just always range_stack.len()?

chinedufn · 2020-04-07T11:17:56Z

src/sections/layer_and_mask_information_section/mod.rs

+                    };
+
+                    let group_id = group_id.unwrap();
+                    let parent_group_id = if group_id > 1 {


Why is this >1 instead of >0? I'm a little confused

chinedufn · 2020-04-07T11:18:45Z

tests/layer_and_mask_information_section.rs


    // Check layer outside group
    psd.layer_by_name("Second Layer").unwrap();
    // Check layer inside group
-    let group = psd.layer_by_name("group").unwrap();
-    group.layers().unwrap().layer_by_name("First Layer").unwrap();
+    psd.group_by_name("group").unwrap();


Since there are two layers can our test make sure that this group contains the correct layer?

chinedufn · 2020-04-07T11:19:18Z

tests/layer_and_mask_information_section.rs


    // Check first group
-    let group = psd.layer_by_name("group").unwrap();
-    group.layers().unwrap().layer_by_name("First Layer").unwrap();
+    psd.group_by_name("group").unwrap();


Same here - can we verify that each group contains the correct layers

chinedufn · 2020-04-07T11:19:49Z

tests/layer_and_mask_information_section.rs

+    assert_eq!(psd.groups().len(), 2);
+
+    // Check group
+    let group = psd.group_by_name("group outside").unwrap();


Same here - can we verify that one group points to no layers and the other group points to one layer

Also can we verify that the child group has its parent group set properly

Something like (just a rough sketch):

let parent = psd.group_by_name("group_inside").unwrap().parent_group().unwrap(); assert_eq!(psd.group_by_id(parent).unwrap().name, "group_outside")`

Otherwise these tests aren't verifying that we're pulling the correct group data - just that we're pulling groups at all

chinedufn

Nice stuff! I think the group parenting logic might be slightly off so I left a note about that

src/sections/layer_and_mask_information_section/mod.rs

chinedufn · 2020-04-07T15:18:39Z

src/sections/layer_and_mask_information_section/mod.rs

        // Read each layer's channel image data
        for (idx, layer_record) in layer_records.into_iter().enumerate() {
-            let group_id = if nested_level > 0 {
+            let group_id = if group_start_stack.len() > 0 {
                Some((groups_count - groups.len() - 1) as u32)


I'm confused by this line. It reads as "Number of groups minus number of groups minus 1". What is this doing?

Layers are present in reverse order. Then, the first layer record contains last layer, so group id will be totally_number_of_groups - already_seen_groups.

Also, ID starts from 0, because it's a index, then -1.

Oh I think I'm following.

So this is leading to GroupDivider::Close opening the folder and GroupDivider::Open closing the folder.

If that's the same - could we just for (idx, layer_record) in layer_records.into_iter().enumerate().reverse() instead of having this inversed logic?

chinedufn · 2020-04-07T15:21:40Z

src/sections/layer_and_mask_information_section/mod.rs

                        end: idx,
                    };

                    let group_id = group_id.unwrap();
-                    let parent_group_id = if nested_level > 0 {
+                    let parent_group_id = if group_start_stack.len() > 0 {
                        Some(group_id - 1)


Could we add a test PSD with the following structure:

Group Group Layer Group Layer

And verify that each of the two sub-groups Group has the correct parent_group_id?

I ask this because right now I think that the logic wouldn't work for this case. It seems like the third parent group will have a parent_group_id of 1 when it should be 0

chinedufn

My only real feedback/questions are around the grouping logic - so I asked a few questions about what you're going for.

Overall the group parsing logic feels a bit more complex than it needs to be - but maybe I'm missing something?

I would've imagined something closer to what you had before with a simple stack that we push to and pop from in order to track the parent group. What changed to lead to removing that approach?

Thanks - nice work!

chinedufn · 2020-04-10T16:58:20Z

src/sections/layer_and_mask_information_section/mod.rs

+                    let frame = Frame {
+                        start_idx: layers.len(),
+                        name: layer_record.name,
+                        group_id: already_viewed,


What is already viewed representing?

chinedufn · 2020-04-10T16:58:36Z

src/sections/layer_and_mask_information_section/mod.rs

@@ -59,6 +59,14 @@ pub struct LayerAndMaskInformationSection {
    pub(crate) groups: NamedItems<PsdGroup>,
 }

+#[derive(Debug)]


What is a frame?

Frame represents a stack frame. It stores all needed data in one struct.

chinedufn · 2020-04-10T16:59:11Z

src/sections/layer_and_mask_information_section/mod.rs

+        let mut layers = NamedItems::with_capacity(layer_count);
+        let mut groups: NamedItems<PsdGroup> = NamedItems::with_capacity(group_count);
+
+        let mut levels: Vec<Vec<Frame>> = vec![];


What are levels?

chinedufn · 2020-04-11T10:49:16Z

src/sections/layer_and_mask_information_section/mod.rs

+        // Read each layer's channel image data
+        for (layer_record, channels) in layer_records.into_iter() {
+            // get current group from stack
+            let current_group_id = stack.last().unwrap().group_id;

            match layer_record.divider_type {
                // open the folder
                Some(GroupDivider::CloseFolder) | Some(GroupDivider::OpenFolder) => {


Why is this both close and open folder?

So, I thought that OpenFolder is open tag, CloseFolder is close tag. It's wrong.
They both are open tags, the difference is, folder is closed or not in layers list.

CloseFolder

OpenFolder

Ohhhhhhh - nice!

Mind just commenting that on their enum definitions?

chinedufn

Feel free to merge when you're ready!

chinedufn · 2020-04-11T18:13:21Z

Published as v0.1.7

Add group layers support

9950764

tdakkota requested a review from chinedufn April 5, 2020 02:58

chinedufn requested changes Apr 5, 2020

View reviewed changes

tdakkota added 2 commits April 7, 2020 09:31

Some suggested changes, add new KeyIDContainer structure, update tests

4d7ee26

Added more test assertions, some fixes

bca5f73

chinedufn requested changes Apr 7, 2020

View reviewed changes

Some suggested changes

a1cf26d

chinedufn requested changes Apr 7, 2020

View reviewed changes

Some fixes, refactor, added tests

a7659b2

chinedufn requested changes Apr 10, 2020

View reviewed changes

Some minor changes

ee900b7

chinedufn reviewed Apr 11, 2020

View reviewed changes

chinedufn approved these changes Apr 11, 2020

View reviewed changes

tdakkota merged commit 18dca33 into chinedufn:master Apr 11, 2020

chinedufn mentioned this pull request Apr 20, 2020

List of supported features #14

Open

Add group layers support #13

Add group layers support #13

Conversation

tdakkota commented Apr 5, 2020

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdakkota commented Apr 6, 2020

chinedufn commented Apr 6, 2020

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

chinedufn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdakkota Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn left a comment

Choose a reason for hiding this comment

chinedufn commented Apr 11, 2020

chinedufn Apr 7, 2020 •

edited

Loading

tdakkota Apr 11, 2020 •

edited

Loading