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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't bother validating class names if --no-css option is passed #78

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@barraponto

The issues jorgebastida#27 and jorgebastida#37 are still around for
files called myfile_1.png and myfile_2.png :/

But instead of fixing that, I'd rather just tell glue not to generate
css. Turns out it will validate the css classes nonetheless. Bad, bad
glue. 馃悰

This PR should fix that behavior.

@jorgebastida

This comment has been minimized.

Show comment
Hide comment
@jorgebastida

jorgebastida Jan 28, 2013

Owner

Thanks for the PR @barraponto,

I've check if the naming issue is still around and I can't reproduce it!
Let's say you have file_1.png, file_2.png, file_3.png and file_4.png inside a folder named sprites and you call glue sprites/ out/, glue will refuse to create the sprite:

$ glue sprites/ out
Processing 'sprites':
Error: Some images will have the same class name:
    sprites/file_1.png => .sprite-sprites-file
    sprites/file_2.png => .sprite-sprites-file
    sprites/file_3.png => .sprite-sprites-file
    sprites/file_4.png => .sprite-sprites-file

That's because glue by default use the filename to determine if you want to add some padding around the image, so having a file named file_4.png means that you want to add 4 pixels around it. http://glue.readthedocs.org/en/latest/paddings.html

If you want to avoid this behaviour, you can use --ignore-filename-paddings.

I think class name validation is an interesting feature and I'm not really sure if I want to remove it from glue as it's glue responsibility generate class names, one of the first things you can expect is that he doesn't generate dups.

Does --ignore-filename-paddings fix your issue?

In the other hand, I'm thinking about moving the _ separator to __ so issues like yours would be harder to reproduce. Probably glue 0.3 is a good version number to do this backward Incompatible change.

Thoughts?

Owner

jorgebastida commented Jan 28, 2013

Thanks for the PR @barraponto,

I've check if the naming issue is still around and I can't reproduce it!
Let's say you have file_1.png, file_2.png, file_3.png and file_4.png inside a folder named sprites and you call glue sprites/ out/, glue will refuse to create the sprite:

$ glue sprites/ out
Processing 'sprites':
Error: Some images will have the same class name:
    sprites/file_1.png => .sprite-sprites-file
    sprites/file_2.png => .sprite-sprites-file
    sprites/file_3.png => .sprite-sprites-file
    sprites/file_4.png => .sprite-sprites-file

That's because glue by default use the filename to determine if you want to add some padding around the image, so having a file named file_4.png means that you want to add 4 pixels around it. http://glue.readthedocs.org/en/latest/paddings.html

If you want to avoid this behaviour, you can use --ignore-filename-paddings.

I think class name validation is an interesting feature and I'm not really sure if I want to remove it from glue as it's glue responsibility generate class names, one of the first things you can expect is that he doesn't generate dups.

Does --ignore-filename-paddings fix your issue?

In the other hand, I'm thinking about moving the _ separator to __ so issues like yours would be harder to reproduce. Probably glue 0.3 is a good version number to do this backward Incompatible change.

Thoughts?

@barraponto

This comment has been minimized.

Show comment
Hide comment
@barraponto

barraponto Jan 28, 2013

Yeah, --ignore-filename-paddings was indeed the issue. But glue's logic first checks for padding, then validates the class name. I believe the latter should be skipped if --no-css is provided.

And 馃嵑 ++ for __padding in a filename. _number is a common pattern when working with tiles and sprites for games (which was actually what I used glue for). It'll be a BC issue though. Nevertheless (and I'll open another PR for this) I believe glue should either -opt-in to this behavior (using --use-filename-paddings) or log it explicitly Using paddings from filenames, --ignore-filename-paddings to disable.

Yeah, --ignore-filename-paddings was indeed the issue. But glue's logic first checks for padding, then validates the class name. I believe the latter should be skipped if --no-css is provided.

And 馃嵑 ++ for __padding in a filename. _number is a common pattern when working with tiles and sprites for games (which was actually what I used glue for). It'll be a BC issue though. Nevertheless (and I'll open another PR for this) I believe glue should either -opt-in to this behavior (using --use-filename-paddings) or log it explicitly Using paddings from filenames, --ignore-filename-paddings to disable.

@jorgebastida

This comment has been minimized.

Show comment
Hide comment
@jorgebastida

jorgebastida Jan 28, 2013

Owner

I think there is a biggest "issue" on the table. You are not the first one using glue to sprite game sprites for html5 games or even iOS. I think I should give a try to "reporters" #64 and make glue a "general purpose" sprite generator.

Some example formats:

  • Plain: Merge images and don't generate any other output.
  • CSS (default): What glue is today
  • JSON: Craft a dictionary-like structure {filename: {x: 44, y:22, width: 120, height:80}, filename2: ... }
  • Cocos2d: http://www.cocos2d-iphone.org/
  • Sparrow: http://gamua.com/
  • ...

I need to think about because it'll require a mayor rewrite, but I think it's a good idea :)

Owner

jorgebastida commented Jan 28, 2013

I think there is a biggest "issue" on the table. You are not the first one using glue to sprite game sprites for html5 games or even iOS. I think I should give a try to "reporters" #64 and make glue a "general purpose" sprite generator.

Some example formats:

  • Plain: Merge images and don't generate any other output.
  • CSS (default): What glue is today
  • JSON: Craft a dictionary-like structure {filename: {x: 44, y:22, width: 120, height:80}, filename2: ... }
  • Cocos2d: http://www.cocos2d-iphone.org/
  • Sparrow: http://gamua.com/
  • ...

I need to think about because it'll require a mayor rewrite, but I think it's a good idea :)

@barraponto

This comment has been minimized.

Show comment
Hide comment
@barraponto

barraponto Jan 29, 2013

Yeah, JSON output would be awesome, although crafty has its own pattern so make sure it's pluggable.
Meanwhile, can we just disable CSS classes validation when --no-css is passed?

Yeah, JSON output would be awesome, although crafty has its own pattern so make sure it's pluggable.
Meanwhile, can we just disable CSS classes validation when --no-css is passed?

@jorgebastida

This comment has been minimized.

Show comment
Hide comment
@jorgebastida

jorgebastida Dec 10, 2013

Owner

Several notes about this issue and glue 0.9:

  • json is now an output format.
  • __ is the default separator for filename-configuration.
  • per-image-filename padding configuration is deprecated.
  • If --no-css is present, no css validation is executed.

As glue 0.9 codebase has changed dramatically, I can't merge this Pull Request, but I'm going to add you @barraponto to the AUTHORS file as you deserve the credit of this fix. Closing this.

Owner

jorgebastida commented Dec 10, 2013

Several notes about this issue and glue 0.9:

  • json is now an output format.
  • __ is the default separator for filename-configuration.
  • per-image-filename padding configuration is deprecated.
  • If --no-css is present, no css validation is executed.

As glue 0.9 codebase has changed dramatically, I can't merge this Pull Request, but I'm going to add you @barraponto to the AUTHORS file as you deserve the credit of this fix. Closing this.

@barraponto

This comment has been minimized.

Show comment
Hide comment
@barraponto

barraponto Dec 11, 2013

I've contributed to several projects before, but this is the first project I make it to the AUTHORS file. 馃嵏

I've contributed to several projects before, but this is the first project I make it to the AUTHORS file. 馃嵏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment