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

Updated demo and removed call to 'mathutils.geometry'. #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

costika1234
Copy link
Contributor

I would ideally like to get rid of the mathutils dependency at some point, so I've just replaced the call to mathutils.geometry.intersect_point_line with a simple Python function called _intersect_point_line. I've also updated the demo with a new floor plan to better test the program and added a quick benchmark function. There is virtually no difference between the 2 implementations (on my machine it took about 10 seconds in each case).

roof demo

@costika1234
Copy link
Contributor Author

@vvoovv: Could you please have a look at this PR? Thanks.

@costika1234
Copy link
Contributor Author

Note that it is more expensive to compute the magnitude of a vector via mathutils than to rely on the simple math.sqrt(x * x + y * y) approach. This can be proved with the following code snippet:

import math
import mathutils
import time

start = time.perf_counter()

for i in range(0, 3000):
    for j in range(0, 3000):
        _ = mathutils.Vector((i, j)).magnitude
        # _ = math.sqrt(i * i + j * j)

end = time.perf_counter()
duration = end - start

print(f'Execution time: {round(duration, 4)}')

The mathutils approach takes about 1.9-2 seconds on my machine, while the obvious math.sqrt solution is consistently between 1.6-1.7 seconds in terms of execution time. I don't see any reason to continue using mathutils (which is also painful to install on a Windows machine that's not intended for development work).

@vvoovv
Copy link
Member

vvoovv commented Sep 8, 2023

Hi @costika1234,

Thank you for your contributions.

I'd prefer to keep mathutils, since we rely on it in the Blosm addon for Blender.

The installation of mathutils is quite easy:

pip install mathutils

@costika1234
Copy link
Contributor Author

It is easy if you have a machine intended for development work (which most likely will have whatever libraries mathutils requires). I had to install 5GB of nonsense in order to get mathutils to work on a Windows machine, which is quite pathetic really... I will go ahead and rewrite this project anyway because I feel like it needs to be library-free. I also think that your Blender integration will not be affected by this change. As long as the tests pass, everything should be fine.

@costika1234
Copy link
Contributor Author

I have completely rewritten this library and managed to get rid of the mathutils dependency: https://github.com/costika1234/bpypolyskel/tree/master. That package can still be used for performance-critical applications without having to change any of the source code.

That being said, I'm not sure if I should open a PR with these changes as it's going to be absolutely huge. I've reformatted all files since there were many style inconsistencies, yet I didn't change any of the original logic.

@vvoovv
Copy link
Member

vvoovv commented Sep 13, 2023

I'd suggest to keep it as a separate fork.

@vvoovv
Copy link
Member

vvoovv commented Sep 13, 2023

May I know how you are using the bpypolyskel library? You can reply to the email prokitektura+support@gmail.com

@costika1234
Copy link
Contributor Author

I have already mentioned this here: #9 (comment). 😉

@vvoovv
Copy link
Member

vvoovv commented Sep 14, 2023

I have already mentioned this here: #9 (comment). 😉

My bad!

@vvoovv
Copy link
Member

vvoovv commented Sep 14, 2023

My concern is about licensing.

Bpypolyskel is released under the GPL license. I hope that you use Bpypolyskel in a way that doesn't contradict the GPL license.

@costika1234
Copy link
Contributor Author

costika1234 commented Sep 14, 2023

I'm not distributing that SketchUp extension as it's just for personal use.

That being said, this PR also addresses a bug related to the computation of the distance between a point and a line (which in the original code was handled by mathutils.geometry.intersect_point_line). The function using that would incorrectly compute the distance due to precision errors when taking the difference of mathutils.Vectors:

>>> from mathutils import Vector
>>> Vector((1, 2.000001)) - Vector((0, 1))
Vector((1.0, 1.0000009536743164))

This is the line of code that would cause this issue: https://github.com/prochitecture/bpypolyskel/blob/master/bpypolyskel/bpyeuclid.py#L93.

In my fork I've actually replaced the analytical approach from this PR with the standard formula for computing the distance between a line and a point. Both methods yield the correct distance (up to really tiny precision errors). The current implementation, on the other hand, produces worse results in some configurations.

@costika1234
Copy link
Contributor Author

Can you acknowledge the issue described in my comment above @vvoovv ? Thanks!

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