vagran / dxf-viewer

DXF 2D viewer written in JavaScript
Mozilla Public License 2.0
295 stars 87 forks source link

[#36] Implement patternFillCalculator for hatch #37

Closed dotoritos-kim closed 1 year ago

dotoritos-kim commented 1 year ago
dotoritos-kim commented 1 year ago

Introducing jest will be next PR

vagran commented 1 year ago

dxf-viewer currently heavily relies on three.js for vector math. Shouldn't its Vector2 class be used here for better readability? three.js lacks Matrix2 but I would introduce one in dxf-viewer (in a separate file), probably will be useful for future features. Just with minimal methods required exactly for this file right now, but compatible in names with three.js Matrix3, e.g. , .invert()., .multiply(), etc.

If you feel it currently does not worth the time, we can leave it as is. I can polish it later.

Phryxia commented 1 year ago

dxf-viewer currently heavily relies on three.js for vector math. Shouldn't its Vector2 class be used here for better readability? three.js lacks Matrix2 but I would introduce one in dxf-viewer (in a separate file), probably will be useful for future features. Just with minimal methods required exactly for this file right now, but compatible in names with three.js Matrix3, e.g. , .invert()., .multiply(), etc.

If you feel it currently does not worth the time, we can leave it as is. I can polish it later.

@vagran I'm the author of this PR (=@kimkanghyune) I'll do that but this week I can't proceed due to my company's situation, so please wait for modification. Thank you for your kind feedback and patience!

dotoritos-kim commented 1 year ago

@vagran

Shouldn't its Vector2 class be used here for better readability?

Polishing is finished! Please verify the changes.

vagran commented 1 year ago

Just noticed - you have used 2 space for indentation, however the project uses 4. I have fixed this in dev-hatch branch.