search

musselwhizzle / Focus-Points

Plugin for Lightroom to show which focus point was active in the camera when a photo was taken
Apache License 2.0
344 stars 42 forks source link

Changes to images for focus point display: #106

Closed philmoz closed 7 years ago

philmoz commented 7 years ago

The rotated & mirrored center images were redundant, replaced these with single images. Only the red center images were used so removed the black, grey and yellow ones. Since the corner images are symmetrical, the mirrored corner images were duplicated by the rotated images. The code now selects an alternate rotation angle if mirroring is needed. The source images and build script don't need to be in the plugin directory, I moved these to the top level of the project. Created sub-directories for the various images instead of having them all in one directory.

musselwhizzle commented 7 years ago

PRs this big, Github has a really hard time showing the diff and the browser locks up on me. Help me some out:

I see no changes to code.

  1. Are you suggesting all deleted assets and not used?
  2. If you moved images to subdirectories, there should be changes in code to reflect the path updates, right?
  3. If mirrored and rotated are redundant, where's the code changes to account for this?
philmoz commented 7 years ago

Both DefaultDelegates.lua and DefaultPointRenderer.lua were updated to accomodate the file name/location changes.

I use SourceTree - it provides a better UI.

rderimay commented 7 years ago

I explained the redundancy of the images in the PR a few days ago. They are redundant in the current chosen design and may not be redundant in a future design. We may very well want to have completely new corner and center designs in the future. Think Face Recognition etc. Given the small amount of place these images are wasting right now, I don't see the need to reduce the universality of the current code just to win 50 K.

philmoz commented 7 years ago

50K? On my Mac this change reduces the plugin size by 6MB and removes 1,444 redundant image files!

It does not preclude adding non-symmetrical, mirrored images in future should the need ever arise - the additional code required to support this would be quite simple.

rderimay commented 7 years ago

Why not just move the currently unneeded icon sets to another folder outside of the .lrdevplugin ? This will remove the most part of the unneeded stuff and will not remove a feature which we could use soon.

philmoz commented 7 years ago

The source images and script have been moved to the top level build/imgs folder (out of the plugin). Everything in the assets/imgs folder is created from these source images - why keep something that can be re-created at any time? Why keep redundant images when a few lines of code can take care of them and produce the same result?

rderimay commented 7 years ago

The point is that it happens that the current images are symmetrical. I dont see any reason to change a universal script to a particular script that works only because of the current particularity of the images. And the size problem is to me a non issue, 6 MB being 1/3th to 1/5th of what only one image weights nowadays.

musselwhizzle commented 7 years ago

Glad to see you guys are discussing reasonably. If an agreed upon decision doesn't happen, I'll weigh in in a couple days.

philmoz commented 7 years ago

I suspect the mirrorred/rotated icons are the main sticking point, so lets break it down into pieces.

Which of the following are you happy with:

  1. move the source images and build script out of the plugin folder
  2. create sub-directories in assets/imgs to group the various image types
  3. remove the unused black, yellow and grey center dot images from the build script & assets folder
  4. condense the 288 red center dot images down to just the 2 images needed, with appropriate code changes
  5. take advantage of the symmetry of the current corner images to remove the 720 corner mirrored images, with code changes to support this

If you really want to keep the mirrored corner images, I don't have a problem with that. I can't envision a scenario where the red center dot would need to be replaced with something more complex so I think point 4 makes sense as well.

rderimay commented 7 years ago

ok. So I weighted back and forth other ways of reducing the number of images while retaining the universality. I went the way to integrate the image name calculation function to the template table. This works, but because of lua's lack of support for inline ifs, this is just hideous and really not a better solution than yours. So, I am for merging this PR as is.

musselwhizzle commented 7 years ago

Thanks guys. Here's what I'm going to do as well: I will make a git tag as the code stands today as stable-001 release. If minds ever change, git will have all the history of the code and files. It will be very easy to revert this out.

musselwhizzle commented 7 years ago

in branch called "asset_reduction" until I can QA. I will merge it over from there tomorrow (or the next day)