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

Add Raya 1-5 files #10

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

Conversation

mnedeljko
Copy link

Add Raya Series puzzles to Repository. I have no idea what I am doing here Aaron but maybe this will work.

Add Raya Series puzzles to Repository
Copy link
Owner

@aaron-siegel aaron-siegel left a comment

Choose a reason for hiding this comment

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

The PR worked, and this is a great start. Thanks!

I left a bunch of code review comments. (I only reviewed Raya 1, but many of the same comments apply to the others, with the obvious parallels.)

src/main/scad/yavuz-demirhan/Raya1.scad Outdated Show resolved Hide resolved
@@ -0,0 +1,66 @@
include <puzzlecad2.scad>

box_puzzle_scale = 15;
Copy link
Owner

Choose a reason for hiding this comment

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

I've been using a scale of 17 for this type of puzzle; I'd suggest we standardize on that for consistency

far = dimlong - mid;
far2 = dimwide - mid;

pieces();
Copy link
Owner

Choose a reason for hiding this comment

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

This should have a * in front of it as well.

src/main/scad/yavuz-demirhan/Raya1.scad Outdated Show resolved Hide resolved

box_puzzle_scale = 15;
box_puzzle_border = 6;
$burr_inset = 0.16;
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason for going with 0.16 mm inset rather than 0.125 (as in other packing puzzles)? Was the smaller inset too tight on your prints?

(I'm not necessarily objecting to a larger inset, but we should be consistent... i.e. use the same value here as in Mushkila and the other packing puzzles, unless there's a good reason they should be different)

Copy link
Author

Choose a reason for hiding this comment

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

Hi Aaron, on my printer 0.125 seems to be too tight for packing puzzles but I agree it should be standard. I will adjust to standard.

Copy link
Owner

Choose a reason for hiding this comment

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

hmm. I have gotten reports from other users that default tolerances are too tight.. it seems there is a lot of variation in tolerances by printer. I am thinking it might make sense to distribute two versions of every puzzle with different tolerances (as different STLs, built from the same .scad)...

Comment on lines 6 to 8
long = box_puzzle_scale * 4; // voxels long
wide = box_puzzle_scale * 3; // voxels wide
high = box_puzzle_scale * 2; // voxels high
Copy link
Owner

Choose a reason for hiding this comment

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

This nomenclature seems a little confusing (there are two sets of dimensions defined with similar names). My suggestion is to declare like so:

interior_dim = [4, 3, 2] * box_puzzle_scale;

Then you can use interior_dim.x, interior_dim.y, and interior_dim.z to refer to the individual coordinate dimensions, if needed.

Comment on lines 10 to 12
dimlong = long + box_puzzle_border * 2;
dimwide = wide + box_puzzle_border * 2;
height = high + box_puzzle_border;
Copy link
Owner

Choose a reason for hiding this comment

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

If you follow my suggestion above, this can become

dim = interior_dim + [box_puzzle_border * 2, box_puzzle_border * 2, box_puzzle_border];

Then dim.x, dim.y, dim.z will refer to the outer dimensions.

Copy link
Author

Choose a reason for hiding this comment

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

Cool, I did not know dimensions could be done as vectors. This is much better.

burr_plate([
["xx|xx"], //piece A
["x..|xxx"],//piece B
], $burr_scale = box_puzzle_scale,$plate_width = 150,$burr_bevel = 1.0);
Copy link
Owner

Choose a reason for hiding this comment

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

code style: add a space after the commas (also applies below)

src/main/scad/yavuz-demirhan/Raya1.scad Outdated Show resolved Hide resolved
Aaron, i updated Raya 1 based on your commenets. How does this look?
@mnedeljko
Copy link
Author

Aaaron I updated Raya1 based on your feedback and removed Raya2-4 until I can update those. I will add them back when complete.

Removed unneccessary comments.
Copy link
Owner

@aaron-siegel aaron-siegel left a comment

Choose a reason for hiding this comment

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

I put in a few more comments. Also, I'd suggest changing the name of the file to "demirhan.raya-1.scad" rather than "Raya1b.scad", to follow the naming convention used elsewhere in the puzzlecad repo.

Thanks!

Comment on lines +17 to +18
box();
cap();
Copy link
Owner

Choose a reason for hiding this comment

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

These should be starred by default

Comment on lines +22 to +24
["xx|xx"],["xx|xx"],["xx|xx"], //piece A-3 copies
["x..|xxx"],["x..|xxx"],["x..|xxx"], //piece B-3 copies
], $burr_scale = box_puzzle_scale,$plate_width = 150,$burr_bevel = 1.0);
Copy link
Owner

Choose a reason for hiding this comment

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

code style: best is to put a space after each comma, for readability


difference() {
beveled_cube(dim);
translate([box_puzzle_border - $burr_inset, box_puzzle_border - $burr_inset, box_puzzle_border- $burr_inset])
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: add an extra $burr_inset on the z axis here:

translate([box_puzzle_border - $burr_inset, box_puzzle_border - $burr_inset, box_puzzle_border - 2 * $burr_inset])

This will provide a little more freedom of movement under the cap.

cube([
interior_dim.x + $burr_inset * 2,
interior_dim.y + $burr_inset * 2,
interior_dim.z + $burr_inset * 2]);
Copy link
Owner

Choose a reason for hiding this comment

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

if you adopt the last suggestion, you'll want to enlarge the z axis here by a tiny amount:

interior_dim.z + $burr_inset * 2 + 0.001

this helps prevent rendering errors that can happen when two faces are exactly coincident in a difference() operation.

Comment on lines +46 to +54
translate([0,0,dim.z])
difference() {
beveled_cube([dim.x, dim.y, box_puzzle_border]);
//first cutout removal
translate([box_puzzle_border+.5*box_puzzle_scale-$burr_inset,box_puzzle_border - $burr_inset,0])
cube([box_puzzle_scale*3+ $burr_inset * 2,box_puzzle_scale*2 + $burr_inset * 2,,box_puzzle_border]);
// second cutout removal
translate([box_puzzle_border+1.5*box_puzzle_scale-$burr_inset,box_puzzle_border-$burr_inset +2*box_puzzle_scale,0])
cube([box_puzzle_scale+2*$burr_inset,box_puzzle_scale+2*$burr_inset,box_puzzle_border]);
Copy link
Owner

Choose a reason for hiding this comment

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

code style: for readability, best to put spaces after commas and between arithmetic operators, e.g.

5, x + y

rather than

5,x+y

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants