The support for Android "dp" unit #158
Conversation
|
Oh, that's a cool idea! And I like that it comes with code :) For now, I have a question: given how "50x50in" right now means "50px x 50in", I would expect "50x50dp" to mean "50px x 50dp", i.e. only the height would be adapted to the device. Since your README uses the mixed unit example, I'm curious whether you interpreted this differently? Also, just so I understand the intention (your plugin may already work like that): If I have a layer of size 100x100 in a document with, say, 120dpi, what is "100dp x 100dp" on a screen with 240dpi? Should the layer be scaled to "200px x 200px"? In the exported image's meta data, should it then mention 240dpi or 120dpi? 👍 Thank you for the first community pull request to the Image Assets plugin! 👻 |
|
Glad to be the first! I consider "50x50dp" as "50dp x 50dp" and this is how my code currently behaves. Not sure about this sort of mixed unit in other scenarios but for Android, I can't think of a use case where you'd need "50px x 50dp". That seems to just open a door for mistakes. On Android, the point of "dp" is to make the image roughly the same physical size on screens with different densities (i.e. dpi). 100dp = 100px on a 160dpi screen. So on a 240dpi screen, 100dp = 100 x 240/160 = 150px. My code just takes the dp number and multiply by the scale factor of the screen density (e.g. 240/160). I'm not familiar though about how the dpi works in images. Would a 100px x 100px, 120dpi image have a different number of pixels than a 100px x 100px, 240dpi image? |
|
@timriot This requires your input. Currently we have the definition that a length without a unit is meant to be in pixels. So currently "50x50in" would be equivalent to "50px x 50in". The approach in this pull request (for dp units) makes "50x50in" equivalent to "50in x 50in". That mirrors how you might say it ("fifty by fifty inches"), which I like. I would describe that approach as "the width unit defaults to the height unit, and the height unit defaults to pixels". However, by that logic "50in x 50" would be equivalent to "50in x 50px" still. Instead we could say "if no unit is specified for either width or height, assume pixels. If a unit is specified for only one of width and height, use the same unit for the other one. If units are specified for both width and height, use those." Then "50in x 50" would be equivalent to "50 x 50in" and "50in x 50in". The thing is, I want the behavior to be consistent across the units. So either we change the behavior of this pull request, or we change the behavior for the other units. That would be a breaking change. |
| if (!fs.existsSync(p)) { | ||
| return mkdirpQ(p); | ||
| } else { | ||
| return; |
There was a problem hiding this comment.
The "else { return; }" is unnecessary.
|
@DennisKehrig I like "if no unit is specified for either width or height, assume pixels. If a unit is specified for only one of width and height, use the same unit for the other one. If units are specified for both width and height, use those." |
| var dirname = require("path").dirname; | ||
| var childPath = dirname(fileName); | ||
| var p = resolve(documentContext.assetGenerationDir, childPath); | ||
| if (!fs.existsSync(p)) { |
There was a problem hiding this comment.
We will also need some code that takes care of deleting these directories again once all images with "dp" units are deleted. Basically, the code that deletes generated files should also try to delete intermediate directories between the assets directory and the image. That would then also come in handy should we ever allow things like "foo/bar.png" as asset names.
|
@lintonye About the DPI of images: changing an images density doesn't change its number of pixels, it merely describes how to convert pixels into physical measurements, for example when printing (a 100x100 image with 100 dpi will be 1in x 1in on paper just like a 300x300 image with 300 dpi, while a 100x100 image with 300 dpi will be 1/3 inches wide and tall). I'm trying to get my thinking straight, so please bear with me for a second... thinks Does that seem correct? If I understand you correctly, right now you basically assume the document is at 160dpi? |
|
@lintonye One concern of mine is that dp units might not stay Android only, possibly resulting in conflicting definitions of dp. Even if another platform uses the same size (160 dp = 1 inch), it might use different density classes (hdpi/ldpi/mdpi/...), or a different convention for the folder names (drawable-hdpi). Android itself may also add another class at some point (maybe xxxhdpi for Qualcomm's 577 dpi Mirasol display). Consequently I would like a more flexible approach. I'm not sure yet what that should look like. Various ideas for brainstorming, in order of increasing craziness:
@timriot Any thoughts? :) |
|
@lintonye Thank you, definitely, for putting up the pull request. I hope you don't feel discouraged by all the comments! I hope you understand that we cannot integrate this as it is. However, this is a very nice challenge for the Generator architecture. One much easier approach might be to use your code as a prototype for a Generator plugin that modifies the behavior of an existing plugin (the Image Assets plugin). I.e., we would add some hooks to the Image Assets plugin that you could exploit. People who need Android dp support would need to install your plugin once (in the 3rd party Plug-Ins/Generator folder) and that's it, or (probably better to avoid conflicts) would need to also enable it per document. This way Adobe doesn't need to make a grand decision here, but users can decide for themselves what they want. How do you feel about any of this? Can you understand my concerns? Would you be interested in working all these things out or would this soon stop being worth the time? Thank you for your feedback! -- Dennis |
|
@DennisKehrig no worries! I'm impressed by your thoroughness and I think I can understand your concerns. I'm happy to continue to contribute as much as my schedule allows. Given the potential ambiguity of "dp" (whereas there's no such ambiguity for "in" or "cm"), it seems to be the easiest approach to have my code as an extension of the Image Assets plugin (plugin of plugin?). Maybe there could be an API to let 3rd party plugins define a new unit? _generator.getPlugin("assets").addUnit("dp", function(component) {
...
}); Keep me posted of how this architecture would evolve! As for the dpi metadata in the image, I did a quick test and Android doesn't seem to care about it at all. A 200px x 200px, 72dpi image in "drawable-hdpi" renders exactly the same physical size as an 200px x 200px, 200dpi image. I think it's safe to just keep the original dpi metadata and scale the image regardless of it. |
|
@lintonye That is wonderful to hear! Thank you for your reassurance. :) One variant of the plugin-of-a-plugin approach would be to move some aspects of the Image Assets plugin to the core, make it extensible, and redesign the Image Assets plugin to work on top of that. For instance, parsing layer names is a generic pattern that other plugins should be able to hook into. Doing this well will take some time that we don't have for this sprint, so I opened a ticket for it (#161). With regards to the DPI metadata, it's not about whether Android respects it (it shouldn't, since the folder name basically declares the DPI), it's about whether we should respect the DPI of the source image. Basically, every image in Photoshop already has a physical size (as in inches, not pixels). Scaling an image to "100x100dp" basically means scaling it to another physical size, and I feel like the original physical size has to be taken into account for that, rather than just looking the amount of pixels that would be necessary to achieve the physical target size. Imagine the same document with the same layers and pixel dimensions, but one has 100 dpi, the other has 200 dpi. I think your plugin would generate the same files out of both, even though the latter is physically half as wide and tall. @timriot This may be a little out of your area, as it relates to mobile development - what would you as designer expect to happen here? Let me know if you need more background information. |
|
Poking @timriot, hoping for an enraged "WHAT?" |
When the user specify "dp" unit, e.g. "48x48dp ic_launcher.png", create different images for different densities in corresponding "drawable-" directories. See this Stackoverflow thread for details about the Android "dp" unit.