nophead / NopSCADlib

Library of parts modelled in OpenSCAD and a framework for making projects
GNU General Public License v3.0
1.21k stars 159 forks source link

HT pipes are broken, due to rework of original code ;) #288

Closed nerdyjan closed 6 days ago

nerdyjan commented 1 week ago

see attached image, before your rework of the HT pipes the lower end of the t-fitting is a pipe, now it'S just a fitting. The t fittings are ok, but the pipes missing the "pipe" part ... the fitting is working fine.

image

nophead commented 6 days ago

Ah I didn't realise the fitting was used on its own. It had a 30mm pipe on the bottom that caused a problem on the T junction.

image

I will put that back with a default length of 30 and pass the tube radius to it in the T junction.

nophead commented 6 days ago

Addressed by 8cdc04b87ba2ed06918e9f3be858abd7962708c1

nophead commented 6 days ago

The fitting should really have a test if it is used on its own.

nerdyjan commented 6 days ago

no, the fitting is never used by it's own. It's always the top (or side) of any Pipe/Junction/etc.

I just checked out your merge and tested it on my project and the fittings in my screenshot are pipes, not just fittings and the error was/is still persistent. i checked out my original code and the screenshots show you, what the problem is.

Your code rework is quite comprehensive and i don't know all the small tips & tricks included in your library. this is why it would cost me to much time to reinvestigate, where the problem occures and why.

can you explain, why my original "not so smart" code was not appropriate? Your rework is quite comprehensive ;)

DAMAGED: image

OK with original code: image

nerdyjan commented 6 days ago

and this is the exploded view (with my original code): image

nerdyjan commented 6 days ago

and this exploded with the current state of the code (after your rework): image

nophead commented 6 days ago

I thought I had explained why I changed the code. Basically I don't have any 3D differences or intersections unless I have to and then they are wrapped in render. Union is free in OpenCSG and 2D operations very fast and so is extrude. 3D differences don't scale when there are thousands of them.

I have no idea why the pipes are missing when they are there in the tests. I am out for the day so will look when I get home. Please can you post your code for the broken example.

On Mon, 7 Oct 2024, 11:37 Jan, @.***> wrote:

and this exploded with the current state of the code (after your rework): image.png (view on web) https://github.com/user-attachments/assets/e0d8bcb7-6549-44f4-9e43-d729bdbc950f

— Reply to this email directly, view it on GitHub https://github.com/nophead/NopSCADlib/issues/288#issuecomment-2396552326, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEKHBKOGARFP2MY5D45LYTZ2JP5ZAVCNFSM6AAAAABPOY6B2GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJWGU2TEMZSGY . You are receiving this because you commented.Message ID: @.***>

nerdyjan commented 6 days ago

I understand ... mainly because of performance ... OK

The pipes are also missing when i run tests.py on your version.

ht_pipes

The code i used for my "case" is:

module case_assembly()
    pose(a = [120, 0, 120], t = [0, 0, 0])
    assembly("case") {
        stl_colour(pp2_colour, view_transparency_case) {
            explode(180)
                translate([0, 0, 12])
                    ht_pipe(HT_50_pipe_250);

                translate([0, 0, -100])
                    rotate([0, 0, 180])
                        ht_tpipe(HT_50_40_tpipe);

            explode(-180)
                translate([0, 0, -360])
                    ht_pipe(HT_50_pipe_250);

        }
    }
nophead commented 6 days ago

Your code works for me once I included lib.scad and fixed lib.scad to include ht_pipes.scad instead of using it.

image I think the bug is an extra unnamed parameter in the call to tube in ht_pipe. That seems to be silently ignored with the version of OpenSCAD I am using but seems to override the h parameter in the version of OpenSCAd you are using. I will push a fix this evening.

nerdyjan commented 6 days ago

I'm using OpenSCAD version 2021.01 on linux. Your guess may hit.

nophead commented 6 days ago

Hopefully 0989a02d95ed26d137238f3866e8b37c9bf5f2e0 fixes it. I reverted the last change.

nerdyjan commented 6 days ago

Great job! Now it works like intended. Thanks and i will study your changes to write better contributions in the future.