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

Use layer index to differentiate between layers with the same name #3492

Merged
merged 4 commits into from Sep 20, 2022

Conversation

martincapello
Copy link
Member

Fix #2656

@martincapello martincapello force-pushed the fix-export-layers-same-name branch 2 times, most recently from 1921443 to 2229b5b Compare August 30, 2022 13:17
@dacap
Copy link
Member

dacap commented Aug 30, 2022

It looks like this change 6a748db includes changes to all end of lines (from Unix-like to Windows-like EOL). Could you please fix that?

@dacap
Copy link
Member

dacap commented Aug 30, 2022

And then 2229b5b does the reverse (?)

@martincapello
Copy link
Member Author

How about now?

@dacap
Copy link
Member

dacap commented Aug 30, 2022

I'll merge the commit with a minor change in the message (adding a missing reference to #2656). And then I'll add some extra changes (because the "- 2" is hard-coded and I would prefer constant defined in layer_frame_comboboxes.h)

@dacap
Copy link
Member

dacap commented Aug 30, 2022

Also two things: 1) I have to check if the index is from top to bottom or bottom to top, but maybe 0 should be the background (bottom), and going up from there. 2) from what it looks, if we specified the layerIndex, we have to specify the layer name too in ExportSpriteSheetParams.

I'll try to fix these issues.

@dacap
Copy link
Member

dacap commented Aug 30, 2022

@martincapello I have pushed some changes into this branch: https://github.com/dacap/aseprite/tree/fix-export-layers-same-name2 but I'm not going to merge this yet as ExportSpriteSheet doesn't remember the last layer exported correctly (to do so we'd need an extra value in pref.xml:

<option id="layer" type="std::string" />
)

If you want to continue from fix-export-layers-same-name2 you can do so. We'll merge this for a future version.

@martincapello
Copy link
Member Author

I'll merge the commit with a minor change in the message (adding a missing reference to #2656). And then I'll add some extra changes (because the "- 2" is hard-coded and I would prefer constant defined in layer_frame_comboboxes.h)

Makes sense. Also, do you think we should have a constant for the -1 returned by the layerIndex() functions?

@martincapello I have pushed some changes into this branch: https://github.com/dacap/aseprite/tree/fix-export-layers-same-name2 but I'm not going to merge this yet as ExportSpriteSheet doesn't remember the last layer exported correctly (to do so we'd need an extra value in pref.xml:

I wasn't sure about adding this extra value to pref.xml, I will do it then.

If you want to continue from fix-export-layers-same-name2 you can do so. We'll merge this for a future version.

Okay, I will continue from that branch. Thanks!

@martincapello
Copy link
Member Author

@dacap Just pushed some changes to let the exporting dialogs remember the last selected layer index.

data/pref.xml Outdated
@@ -485,6 +485,7 @@
<option id="filename" type="std::string" />
<option id="resize_scale" type="double" default="1" />
<option id="layer" type="std::string" />
<option id="layerIndex" type="int" default ="-1" />
Copy link
Member

Choose a reason for hiding this comment

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

Please check that we are using other format in the pref.xml file:

Suggested change
<option id="layerIndex" type="int" default ="-1" />
<option id="layer_index" type="int" default ="-1" />

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that this is not the first time I made this mistake...🤦
Will fix it, thanks!

@dacap dacap self-assigned this Sep 19, 2022
@dacap dacap merged commit 05dd5f8 into main Sep 20, 2022
@dacap dacap deleted the fix-export-layers-same-name branch September 20, 2022 17:26
Comment on lines +230 to +231
if (layer->name() == layerName && layerIndex == -1 ||
layer->name() == layerName && layerIndex == i) {
Copy link
Member

Choose a reason for hiding this comment

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

@martincapello just saw a warning (warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]) about all these conditions, it's already merged but it would be nice to fix it:

      if (layer->name() == layerName && layerIndex == -1 ||
          layer->name() == layerName && layerIndex == i) {

It's equal to:

      if ((layer->name() == layerName) && (layerIndex == -1 || layerIndex == i)) {

The same for the other conditions, must be reviewed (these are 3 times in this PR).

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.

Export Sprite Sheet cannot differentiate between Layers with matching names
2 participants