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.

Sign up for GitHub

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

Cardinal Directions for Roofs and Misc Roof Fixes #61

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Cardinal Directions for Roofs and Misc Roof Fixes #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c8bb76
Remove Extra 'round' Roof Type
justinormont Jul 7, 2024
5adf249
Remove extranious round roof from list; add cardinal directions to ro…
justinormont Jul 7, 2024
a4d3588
Update main.yml
Beakerboy Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: CI
on: push
on: [push, pull_request]
jobs:
build:
runs-on: ubuntu-latest
Expand Down
62 changes: 56 additions & 6 deletions src/buildingpart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ class BuildingPart {
// Should Skillion be included here?
const directionalRoofs = ['gabled', 'round'];
calculatedOptions.roof.direction = this.options.specified.roof.direction ?? this.options.inherited.roof.direction;
if (!calculatedOptions.roof.direction && directionalRoofs.includes(calculatedOptions.roof.shape)) {
if (calculatedOptions.roof.direction === undefined && directionalRoofs.includes(calculatedOptions.roof.shape)) {
// Radians pi > x >= -pi
let longestSide = BuildingShapeUtils.longestSideAngle(this.shape);
if (longestSide < 0) {
Expand Down Expand Up @@ -176,7 +176,7 @@ class BuildingPart {
window.printError('Way ' + this.id + ' is taller than building. (' + this.options.building.height + '>' + this.options.inherited.building.height + ')');
}
// Should skillion automatically calculate a direction perpendicular to the longest outside edge if unspecified?
if (this.options.roof.shape === 'skillion' && !this.options.roof.direction) {
if (this.options.roof.shape === 'skillion' && this.options.roof.direction === undefined) {
window.printError('Part ' + this.id + ' requires a direction. (https://wiki.openstreetmap.org/wiki/Key:roof:direction)');
}
this.extrusionHeight = this.options.building.height - this.options.building.minHeight - this.options.roof.height;
Expand Down Expand Up @@ -388,10 +388,10 @@ class BuildingPart {
* Direction. In degrees, 0-360
*/
static normalizeDirection(direction) {
// if (cardinal) {
// convert to degrees
// return degrees;
// }
const degrees = this.cardinalToDegree(direction);
if (degrees !== undefined) {
return degrees;
}
if (direction) {
return parseFloat(direction);
}
Expand All @@ -411,6 +411,56 @@ class BuildingPart {
* North is zero.
*/
static cardinalToDegree(cardinal) {
const cardinalUpperCase = `${cardinal}`.toUpperCase();
if (cardinalUpperCase === 'N') {
return 0;
}
if (cardinalUpperCase === 'NE') {
return 45;
}
if (cardinalUpperCase === 'E') {
return 90;
}
if (cardinalUpperCase === 'SE') {
return 135;
}
if (cardinalUpperCase === 'S') {
return 180;
}
if (cardinalUpperCase === 'SW') {
return 225;
}
if (cardinalUpperCase === 'W') {
return 270;
}
if (cardinalUpperCase === 'NW') {
return 315;
}
if (cardinalUpperCase === 'NNE') {
return 22;
}
if (cardinalUpperCase === 'ENE') {
return 67;
}
if (cardinalUpperCase === 'ESE') {
return 112;
}
if (cardinalUpperCase === 'SSE') {
return 157;
}
if (cardinalUpperCase === 'SSW') {
return 202;
}
if (cardinalUpperCase === 'WSW') {
return 247;
}
if (cardinalUpperCase === 'WNW') {
return 292;
}
if (cardinalUpperCase === 'NNW') {
return 337;
}
return undefined;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ function createFolders(folder, options) {
if (property === 'colour') {
roofFolder.addColor(options.roof, property);
} else if (property === 'shape') {
const roofTypesAvailable = ['dome', 'flat', 'gabled', 'onion', 'pyramidal', 'skillion', 'hipped', 'round', 'gambrel', 'round'];
const roofTypesAvailable = ['dome', 'flat', 'gabled', 'onion', 'pyramidal', 'skillion', 'hipped', 'round', 'gambrel'];
// If this roof is not supported, add it to the list for sanity.
if (!roofTypesAvailable.includes(options.roof.shape)) {
roofTypesAvailable.push(options.roof.shape);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(pinned to a line of code to allow for threaded discussion)

Beakerboy commented Jul 8, 2024, 7:02 AM PDT
Could you add a test for the new cardinal direction code? And one that demonstrates that a building with a north roof does not produce an entry in the error log, but one with a missing direction does?

I don't see instructions on your up-and-coming contribution guide for running tests. Is there a simple method?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, it’s in the GitHub workflow. They should automatically run when you check in your code.

CI=true NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules" npm test

Isn’t this the standard way to run tests?

Copy link
Owner

Choose a reason for hiding this comment

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

Also. On Wisconsin! I grew up in Wausau and got my Chemical Engineering degree there. Class of 2000.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seems to be some pre-steps needed to locally run the unit test:

% CI=true NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules" npm test

> [email protected] test
> c8 jest

(node:66111) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:66114) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:66113) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:66112) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:66111) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
PASS test/utils.test.js
FAIL test/split_way_multipolygon.test.js
  ● Test suite failed to run

    Cannot find module 'pyramid' from 'src/buildingpart.js'

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)

(node:66113) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
FAIL test/multipolygon.test.js
  ● Test suite failed to run

    Cannot find module 'pyramid' from 'src/buildingpart.js'

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)

(node:66114) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
FAIL test/building.test.js
  ● Test suite failed to run

    Cannot find module 'pyramid' from 'src/buildingpart.js'

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)

(node:66112) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
Test Suites: 3 failed, 1 passed, 4 total
Tests:       9 passed, 9 total
Snapshots:   0 total
Time:        1.546 s
Ran all test suites.

=============================== Coverage summary ===============================
Statements   : 16.56% ( 265/1600 )
Branches     : 66.66% ( 18/27 )
Functions    : 33.33% ( 7/21 )
Lines        : 16.56% ( 265/1600 )
================================================================================

Generally one runs unit tests locally before the CI/CD runs them. Otherwise it can be a long dev test-fail-iterate cycle.

Copy link
Owner

@Beakerboy Beakerboy Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I’m not a real programmer and usually just code on my phone using GitHub as my IDE. I can’t run SSH at work, and when I get home I don’t use my computer, so this is it. The GitHub workflow has the wget or curl statement I use to pull down those other files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(tagged on a line of code to allow for threaded discussion)

Beakerboy commented Jul 18, 2024, 4:28 AM PDT
I submitted a pull request to your branch. Please review it and let me know what you think. I refactored your function to make is shorter and created a unit test for it. It still needs a test for the message in the error log.

I added some feedback to your PR (on my PR) -- justinormont#1

Expand Down