Fix#52

[alextychan]

Purpose / Goal

Patch a fix for #104 and to allow featurePointSize to be edited in each image separately.

Bug cause

Zooming in/out causes the image to be resized. But the shape size was never changed. Rather it was merely transformed by the svg-pan-zoom plugins. Meaning the relative size of the shape never grew/shrunk according to image scale.

Solution

Removed svg-pan-zoom dependencies and rescaled the images manually.

Patch changes

1. config.js
   • rescaled feature point radius based on image scale
   • replaced appConfig.featurePointSize with labellingData[imgSelected.name].featurePointSize
2. store.js
   • added convenience methods to rescale shapes, feature points, shape points, and the shape rbox.
   • made sure all points saved into the store are positioned relative to image size at 100%. e.g. if position (100, 100) @ 50% zoom -> (200, 200) @ 100% zoom.
   • added featurePointSize to each image, this is to give user finer control over featurePointSize in each image
3. workarea.tag.html
   • made sure all points drawn are all scaled according to current image scale by using convenience methods in store.js or by scaling manually.
4. settings.js
   • removed featurePointSize from appConfig
5. landmark-action.tag.html
   • removed dependencies on featurePointSize, replaced with labellingData[imgSelected.name].featurePointSize.

Convenience methods added in store.js

1. scaleShape(id, type, bbox, points, scale)
   • scale shape according to given scale
2. scaleShapePoints(points, scale, type)
   • scale the points making up the shape according to type and scale
   • p.s. I have bad naming sense. :)
3. scaleBbox(bbox, scale)
   • scale the rbox of shape according to scale.
   • Note: I have no clue why it's named bbox, when we are actually providing shape.rbox(myCanvas) as the argument, but I'm using this name to maintain consistency
4. scaleFeaturePoints(featurePoints, scale)
   • scales the feature points of the shape according to scale.

Additional documentation

All the method documentation should be included in the code. Please have a look if you are interested.

svg-pan-zoom suggestions

We could probably still use the plugin for panning and zooming. Instead of applying it to each shape, we could probably just apply it over the whole canvas if we wanted to. Then by enabling the scroll to zoom, this would allow the user to zoom in/out at the user cursor position. Haven't explored that route yet, but has good potential.

Thanks for reading all the way.

Type

Please mention the type of PR

• [x] Bug Fix
• [ ] Refactoring / Technology upgrade
• [ ] New Feature
• [ ] Documentation
• [ ] Other : | Please Specify |

Note : Please ensure that you've read contribution guidelines before raising this PR.

[alextychan]

@amitguptagwl, @Alvin-Voo

[alextychan]

Additional comments

The pasted shape is relative to the when it was copied. Say shape was copied @ 50% zoom, then during paste, the shape size will always be equal to the shape size @ 50% and is irrelevant to current shape size.

[amitguptagwl]

Thanks @alextychan for the PR. I'll take some time to review the changes. And planning to merge it by Sunday so even @Alvin-Voo will get the chance to cross review it.

[amitguptagwl]

Question: With the changes in landmark-action.tag.html, it looks like we don't want to persist the change in feature point size for all the images but for current image only. How will it be helpful?

[alextychan]

@amitguptagwl
There's actually another reason to change the feature point size such that every image has a control over it.

The main reason was that of feature point scaling issues. Currently, the feature point size is scaled according to the image scale. The following bug would happen if image scale is not taken into account.

1) Draw a shape and add feature points. 2) Zoom out/in. 3) Copy that shape. 4) Switch to a new image. 5) Paste the image.

Expected Result: Shape pasted should have the same feature point size as the image copied. Obtained Result: Feature point size for that pasted shape is larger than the copied shape.

Reason for bug: Feature point size is reliant on image scale. Please have a look at config.js. Feature point size is scaled according to image scale.

    // Feature point size should be based on current image scale
    var featurePointSize = labellingData[imgSelected.name].featurePointSize;
    var point =  container.parent().circle()
        .radius(Math.floor(featurePointSize * imgSelected.size.imageScale))
        .attr({
            cx: position.x - canvasOffset.x - containerOffset.x,
            cy: position.y - canvasOffset.y - containerOffset.y})

So if the original image had a radius of 25. Let's say the zoom was set to 50%, the radius size would become 12.5 due to rescalling. But the actual radius size never changed, because it's a global variable.

If we copied that shape, and pasted it into a new image. The new image scale is 1. But the feature point size still persists because it's a global constant. Now radius for the pasted shape would be 25. And if you change the feature point size now, there would be a consistency bug where the scale is wrong for the pasted shape.

Hence to prevent that bug and to provide better consistency. I believe the change is necessary. Plus, isn't it great to be able to have different sizes for different images? ;)

[alvinvoo]

@alextychan Wow, I spent some time reading through your previous Copy and Paste PR. Thank you for doing such a fantastic job! I can see that you have put in a lot of effort, including this fix. AND, not to mention all those nicely formatted comments, you've really beaten both of us to it.

I have played around with this PR and I think its a great idea to just scale the shapes literally, I saw that you took great pain to include scale in all the functions that call on any shape, well done. So the only issue left is just the feature point size.

One question, who put the featurePointSize * imgSelected.size.imageScale in getPointToDraw? Was it you or @amitguptagwl ?

    var point =  container.parent().circle()
        .radius(Math.floor(featurePointSize * imgSelected.size.imageScale))
        .attr({
            cx: position.x - canvasOffset.x - containerOffset.x,
            cy: position.y - canvasOffset.y - containerOffset.y})

Can we just don't scale the featurePointSize and let it be default to 3? Zoom in and out will see the points remain as same sizes. See my screencast below. Doesn't this look better?

And, what's the motivation for allowing users to be able to zoom in/out the feature points? Is it for visual assistance? If it's only for visual assistance when users are drawing, then I think should be fine by not scaling the points. So, every time users copy and paste the shape to another image, the feature points default to size 3.

But if @amitguptagwl really wants to keep the same feature points size (scale) across all copy and paste, and probably for persistence in future, then every time the scale changes, the size of the feature points need to be stored as well, most probably in scaleShape.

function scaleShape(id, type, bbox, points, scale) {
    return {
        "id" : id,
        "label" : "unlabelled",
        "type" : type,
        "points": scaleShapePoints(points, scale, type),
        "bbox" : scaleBbox(bbox, scale) || {
            "x": 0,
            "y": 0,
            "w": 0,
            "h": 0        },
        "attributes": [],
        "tags": [],
        "featurePoints": [],
        "newFeaturePointSize": xxx,  //<--- here
        "zoomScale" : 1,
        "defaultZoomScale": 1/imgSelected.size.imageScale
    }
}

Then in getPointToDraw of config.js, instead of getting the featurePointSize from labellingData, we can get from each shape's. This essentially means that each shape will have it's own feature point size definition. This will definitely affect landmark-action.tag.. and potentially introduce many more discrepancies.. So, the best way out to me is just to use same size for all; The landmark tool is just for visual assistance only, and the feature points size are not persisted.

Yup, I think we could do away with svg-pan-zoom. If you are proceeding with this PR, please remove setZoomLevel() in workarea and the svg-pan-zoom library file as well.

Lastly, I really think the code are becoming more and more spaghetti like =D. I worked on this before but it still takes me more than 2 hours to familiarize myself again with all the changes. I think we should start commenting like @alextychan not only on new functions but also on all the existing ones that we happen to work or will work on. Once we understand what a function does, we can just put a comment on top.

Other than this, the technical.md is good but too generic. We need a flowchart of how different components of this application interacts with each other. We can look at using https://www.draw.io/ for this purpose. Else, it's very hard for developers to jump on board.

And finally @amitguptagwl , may I know the reason we are not using npm and webpack? =D

[alextychan]

@Alvin-Voo, I was the one that applied image scale to the feature point size. As shown here.

var point =  container.parent().circle()
        .radius(Math.floor(featurePointSize * imgSelected.size.imageScale))
        .attr({
            cx: position.x - canvasOffset.x - containerOffset.x,
            cy: position.y - canvasOffset.y - containerOffset.y})

My main motivation was because I thought that it would be more consistent if we actually scaled feature point size according to image scale. But you made a good point about storing feature point size in each shape. Such that we would be able to persist the change in feature point size. However, it's just a choice of implementation.

The reason I chose to scale feature point size according to the image scale was such that all points would share the same size. But if we ever want to be able to implement a new feature such as "transform/scale", that would be something we have to consider. E.g. during scaling of shape should the feature point size change as well? For now, I think this suffices. If the general consensus is to move the featurePointSize into shape, then I will make the change. As stated here by @Alvin-Voo.

function scaleShape(id, type, bbox, points, scale) {
    return {
        "id" : id,
        "label" : "unlabelled",
        "type" : type,
        "points": scaleShapePoints(points, scale, type),
        "bbox" : scaleBbox(bbox, scale) || {
            "x": 0,
            "y": 0,
            "w": 0,
            "h": 0        },
        "attributes": [],
        "tags": [],
        "featurePoints": [],
        "newFeaturePointSize": xxx,  //<--- here
        "zoomScale" : 1,
        "defaultZoomScale": 1/imgSelected.size.imageScale
    }
}

Lastly, I would like to agree with @Alvin-Voo, that the code is looking like spaghetti code and needs reformatting. We ought to have some guideline to follow, and use some build tools. Since I don't have any knowledge about this, I'll gladly leave the discussion to both you and @amitguptagwl. One thing we could probably use, however, is prettier. https://prettier.io/. It looks promising.

[alextychan]

On a side note, I think it would be great if we have a channel for communication. Something like slack/discord would do wonders.

[amitguptagwl]

Sorry to be late for the party.

@alextychan @Alvin-Voo Feature points or landmark points are good when they're small and visible enough. A small point gives more accuracy but they're hard to find and select. Hence we've given the feature to set their size globally. So that the user can decide what size they're comfortable with. Setting it for each image may be frustrating so we manage it under global configuration. However, keeping it local to each image is another perspective as all the images are not same and an user may not need the same size point across the images.

Yes, we should not scale feature points while we're scaling the images or labels. The purpose of scaling is to label an image more accurately. If we'll increase the size of a feature-point or the border of the shapes then we'll not be able to label an image accurately.

@Alvin-Voo We used npm and npm based riotjs compilation in the starting but I realized that many contributors wanted a simple approach for contribution. Hence removed. You must have noticed that there are many stories which require just 2-minute work but I spend around 15-20 minutes to write the issue. So that first-timers can confidently start with opensource development. So until or unless it is necessary or we've more collaborators, I want to keep tech stack as basic as possible. However, you can raise an issue where we can discuss the tech stack which can be useful for this project and then come up with a simple solution.

No doubt, we should focus on a detailed guideline which can help new developers. draw.io is a good option. but we should have editable designs. so we can either attach draw.io project file or probably some markdown based drawings.

@alextychan I can create the channel anytime if more contributors are interested in that.

Many thanks to @alextychan for you hark work, and clean development and to @Alvin-Voo for your detailed review. And yes we should refactor the code for future development.

@alextychan as I suggested on your last PR, you should create a post about your contribution on steemit.com. If you've never created it before then @Alvin-Voo can guide you.

[alvinvoo]

Okay @amitguptagwl .

Yes, we should not scale feature points while we're scaling the images or labels. The purpose of scaling is to label an image more accurately. If we'll increase the size of a feature-point or the border of the shapes then we'll not be able to label an image accurately.

Then I think for this PR, it would be suffice for @alextychan to remove imageScale from the following code and call it a day?

var point =  container.parent().circle()
        .radius(Math.floor(featurePointSize * imgSelected.size.imageScale))
        .attr({
            cx: position.x - canvasOffset.x - containerOffset.x,
            cy: position.y - canvasOffset.y - containerOffset.y})

Take note that by doing this, the feature points of a shape will take the featurePointSize of current image. So if you are at image 1, your featurePointSize is 5, you zoom in until 120%, you copy a shape and you paste the shape into image 2 where featurePointSize is 10, the featurePointSize of the pasted shape will become 10.

If the featurePointSize is supposed to be unique to each image, then this should be fine. But it might seem a bit awkward (un-intuitive) at first =D

yup, draw.io is a good option. Many people can contribute to a single diagram and the output can be saved as a xml like this https://github.com/StephenGrider/ReactSSRCasts/tree/master/diagrams/01 It can then be versioned in github. The raw xml file can be opened as url as such https://www.draw.io/#Uhttps%3A%2F%2Fraw.githubusercontent.com%2FStephenGrider%2FReactSSRCasts%2Fmaster%2Fdiagrams%2F01%2Fdiagrams.xml So convenient! And of course, you can export it out as image.

[amitguptagwl]

What I meant is the size of the feature point should not be impacted due to the zoom level. So if it is 3 on an image and we paste the shape on another image it should be still 3. We don't want to link the size of feature-point to any image.

[alextychan]

@amitguptagwl, thanks for merging this branch. I have looked at steemit.com and am not in it for the rewards so I'll pass.

So just to reiterate @Alvin-Voo, the feature point size now no longer scales when the image zooms. So feature point size will remain static. When you copy a shape from image A and paste it to image B, the feature point size will be based on image b.

Again, I would like to express my gratitude to @Alvin-Voo for the review and suggestions and @amitguptagwl for the awesome project and guidance.

[amitguptagwl]

Many thanks for your contribution @alextychan