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

Added RouteOpisometer and related changes. #606

Merged
merged 4 commits into from
Mar 12, 2021
Merged

Added RouteOpisometer and related changes. #606

merged 4 commits into from
Mar 12, 2021

Conversation

RichardRobertson
Copy link
Contributor

This new tool lets you draw an opisometer that always sticks to cells that have routes and you can drag it backward to "undo" that part of the route. It works on main roads, trails, and sea routes.

Changes:

Adds RouteOpisometer class based on Opisometer

Adds button to Units dialog to draw new route opisometer
Adds SVG symbol "icon-route" for the button

Adds commented out CSS icon entry for if FontAwesome is updated in the future

I am not very familiar with web development and I strongly dislike JavaScript but I did my best to match your existing code style.

Closes #605

@Azgaar
Copy link
Owner

Azgaar commented Mar 11, 2021

Hello and thank you for the contribution! It's definitely a viable solution. I have added a few minor things to fix (if you don't have time, I can fix on my own). I also believe there should be a validation that added cell is unique, so Set of cells should be used.

I got the idea of measuring routes. But you cannot follow the exact route this way, so I don't see a real reason to limit user for using only cells with some route. It just confuse users. Can we turn it the way to use any cell?

@RichardRobertson
Copy link
Contributor Author

RichardRobertson commented Mar 11, 2021

Sure, I can implement a set of cells to eliminate repeats.

I'm not sure what you mean about not following the exact route. When I was testing it the line was extremely close (with some wobble for curves).
I can add code to make hold shift disable the route requirement so it can draw into any cell. Does that sound good?

@Azgaar
Copy link
Owner

Azgaar commented Mar 12, 2021

I believe that there is no a lot of sense to stick to routes. It can be both ways, shift to stick or shift to not stick.

@RichardRobertson
Copy link
Contributor Author

The idea is that I used this generator to make a world map for my tabletop RPG and measuring the routes allows me to figure out travel time for my adventurers. I didn't change the old opisometer, I just added a second one. The old one can measure anything and this one measures manufactured routes. That's what I was thinking.

Tomorrow I'll add the extra features to my code.

…ts. Also allow holding shift to go off-road while drawing it.
@RichardRobertson
Copy link
Contributor Author

Done. Got the cellStops changed to act like a set (still an array for indexing reasons) and added holding shift to go off-road.

@Azgaar
Copy link
Owner

Azgaar commented Mar 12, 2021

Thanks, would you please also check the review comments? There are a few minor issue and class should be refactored

@RichardRobertson
Copy link
Contributor Author

Where are review comments? I do not see them. On top of this page it says "Reviewers No reviews." Did you publish the review?

icons.css Outdated Show resolved Hide resolved
class RouteOpisometer extends Measurer {
constructor(points) {
super(points);
if (pack.cells) {
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure, in what cases pack.cells is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to fix an error when loading a map. If you make a map, draw a RouteOpisometer, save to storage, set options to load last saved map, and reload the page, pack.cells is not defined at the time when rulers are recreated. I found that error while testing to make sure that it loaded properly again.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
modules/ui/measurers.js Outdated Show resolved Hide resolved
modules/ui/measurers.js Outdated Show resolved Hide resolved
@Azgaar
Copy link
Owner

Azgaar commented Mar 12, 2021

Ok, that was on my side, thanks for checking.

Reverted icons.css to master
Removed index.html/icon-route class and modified size for SVG icon on button
Refactored RouteOpisometer.trackCell so that duplicate code is pulled outside of if blocks
@RichardRobertson
Copy link
Contributor Author

I addressed all the review comments. Thank you for your patience while I worked through this.

@Azgaar
Copy link
Owner

Azgaar commented Mar 12, 2021

thanks for the edits, I will check and merge it soon

@Azgaar
Copy link
Owner

Azgaar commented Mar 12, 2021

Looks good, thanks for the new feature!

@Azgaar Azgaar merged commit 5e079d1 into Azgaar:master Mar 12, 2021
@Azgaar
Copy link
Owner

Azgaar commented Mar 12, 2021

Announced the change on Reddit and Discord so that people know new functional is there.

@RichardRobertson RichardRobertson deleted the Route-Opisometer branch March 12, 2021 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Road distance tool
2 participants