search

fstl-app / fstl

A fast STL file viewer
463 stars 106 forks source link

Wait for the STL file size to stop changing. #107

Closed sur5r closed 1 year ago

sur5r commented 1 year ago

Fixes #37.

It's the approach from #106 but using Qt only.

10 ms was still racy in my tests (writing STL from OpenSCAD), so I went with 100 ms.

DeveloperPaul123 commented 1 year ago

If you could retest from master and see if 10 ms is still racy for you, I'm happy to accept a PR that adjusts the time. Or maybe we can make it configurable

sur5r commented 1 year ago

If you could retest from master and see if 10 ms is still racy for you, I'm happy to accept a PR that adjusts the time. Or maybe we can make it configurable

That was not the main point of this PR. My focus was on using Qt only, not going to POSIX/plain STL, and having less code for the same functionality.

DeveloperPaul123 commented 1 year ago

Ok my mistake. I misunderstood the goal of the PR. I'll give this a test and see if we can merge it in still

DeveloperPaul123 commented 1 year ago

@gsohler Would you be willing to test this PR out and see if it solves #37 for you as well? I think I prefer the Qt approach for fstl in this case

gsohler commented 1 year ago

37 is closed already , suppose the answer was yes

On Fri, May 26, 2023 at 6:34 PM Paul T @.***> wrote:

@gsohler https://github.com/gsohler Would you be willing to test this PR out and see if it solves #37 https://github.com/fstl-app/fstl/issues/37 for you as well? I think I prefer the Qt approach for fstl in this case

— Reply to this email directly, view it on GitHub https://github.com/fstl-app/fstl/pull/107#issuecomment-1564645500, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MV6J7BVJJDZCQBHP6TXIDLRPANCNFSM6AAAAAAYP7GDGQ . You are receiving this because you were mentioned.Message ID: @.***>

DeveloperPaul123 commented 1 year ago

37 is closed already , suppose the answer was yes

… On Fri, May 26, 2023 at 6:34 PM Paul T @.> wrote: @gsohler https://github.com/gsohler Would you be willing to test this PR out and see if it solves #37 <#37> for you as well? I think I prefer the Qt approach for fstl in this case — Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MV6J7BVJJDZCQBHP6TXIDLRPANCNFSM6AAAAAAYP7GDGQ . You are receiving this because you were mentioned.Message ID: @.>

I closed that due to your PR. This PR addresses the same issue but in a different way and does not include the changes you made. In retrospect I think I prefer the Qt-centric approach to solve the issue.

gsohler commented 1 year ago

i am fine with that. i am happy, that my proposed algorithm is still inside.

On Fri, May 26, 2023 at 9:07 PM Paul T @.***> wrote:

37 https://github.com/fstl-app/fstl/issues/37 is closed already ,

suppose the answer was yes … <#m8963716526631296567> On Fri, May 26, 2023 at 6:34 PM Paul T @.> wrote: @gsohler https://github.com/gsohler https://github.com/gsohler https://github.com/gsohler Would you be willing to test this PR out and see if it solves #37 https://github.com/fstl-app/fstl/issues/37 <#37 https://github.com/fstl-app/fstl/issues/37> for you as well? I think I prefer the Qt approach for fstl in this case — Reply to this email directly, view it on GitHub <#107 (comment) https://github.com/fstl-app/fstl/pull/107#issuecomment-1564645500>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MV6J7BVJJDZCQBHP6TXIDLRPANCNFSM6AAAAAAYP7GDGQ https://github.com/notifications/unsubscribe-auth/ACCO4MV6J7BVJJDZCQBHP6TXIDLRPANCNFSM6AAAAAAYP7GDGQ . You are receiving this because you were mentioned.Message ID: @.>

I closed that due to your PR. This PR addresses the same issue but in a different way and does not include the changes you made. In retrospect I think I prefer the Qt-centric approach to solve the issue.

— Reply to this email directly, view it on GitHub https://github.com/fstl-app/fstl/pull/107#issuecomment-1564813661, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MVI6T4CFENQRN4S3OTXID5PXANCNFSM6AAAAAAYP7GDGQ . You are receiving this because you were mentioned.Message ID: @.***>

DeveloperPaul123 commented 1 year ago

i am fine with that. i am happy, that my proposed algorithm is still inside. … On Fri, May 26, 2023 at 9:07 PM Paul T @.> wrote: #37 <#37> is closed already , suppose the answer was yes … <#m8963716526631296567> On Fri, May 26, 2023 at 6:34 PM Paul T @.> wrote: @gsohler https://github.com/gsohler https://github.com/gsohler https://github.com/gsohler Would you be willing to test this PR out and see if it solves #37 <#37> <#37 <#37>> for you as well? I think I prefer the Qt approach for fstl in this case — Reply to this email directly, view it on GitHub <#107 (comment) <#107 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MV6J7BVJJDZCQBHP6TXIDLRPANCNFSM6AAAAAAYP7GDGQ https://github.com/notifications/unsubscribe-auth/ACCO4MV6J7BVJJDZCQBHP6TXIDLRPANCNFSM6AAAAAAYP7GDGQ . You are receiving this because you were mentioned.Message ID: @.> I closed that due to your PR. This PR addresses the same issue but in a different way and does not include the changes you made. In retrospect I think I prefer the Qt-centric approach to solve the issue. — Reply to this email directly, view it on GitHub <#107 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MVI6T4CFENQRN4S3OTXID5PXANCNFSM6AAAAAAYP7GDGQ . You are receiving this because you were mentioned.Message ID: @.>

Thanks for being flexible