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

New offset line node. #1958

Merged
merged 9 commits into from
Dec 23, 2017
Merged

New offset line node. #1958

merged 9 commits into from
Dec 23, 2017

Conversation

Durman
Copy link
Collaborator

@Durman Durman commented Dec 20, 2017

My first node)) not sure that I did all right :/
#1914
image


from mathutils import Vector, Matrix
from math import radians, pi, sin
from mathutils.geometry import normal
Copy link
Collaborator

Choose a reason for hiding this comment

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

stick imports at the top of the file, around the bpy import. See how other nodes do this.

    1. standard python imports, (math, collections... etc)
    1. bpy/blender imports, (mathutils...etc)
    1. sverchok imports (utils..etc)

offset = FloatProperty(
name='offset', description='distance of offset',
default=0.1,
options={'ANIMATABLE'}, update=updateNode)
Copy link
Collaborator

Choose a reason for hiding this comment

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

option={'ANIMATABLE'} is not needed, properties have this attribute set to animatable by default.

if not ((self.outputs['Vers'].is_linked or self.outputs['Faces'].is_linked
or self.outputs['OuterEdges'].is_linked or self.outputs['VersMask'].is_linked)
and (self.inputs['Vers'].is_linked or self.inputs['Edgs'].is_linked)):
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would first test if the input vers and edges is linked. Then test if there's also useful output links.

@zeffii
Copy link
Collaborator

zeffii commented Dec 20, 2017

the goal is to keep the process function abstract, as a "task master".

process is best for

  1. get data
  2. sanitize data into best form
  3. send (sanitized) data to worker functions
  4. get product from worker functions
  5. fill outputs sockets with product.

The worker functions are cohesive blocks of code that are often specialized enough to stick into their own function.

@Durman
Copy link
Collaborator Author

Durman commented Dec 21, 2017

@zeffii Okay, I understood. I will fix.

@Durman
Copy link
Collaborator Author

Durman commented Dec 21, 2017

@zeffii

i was only thinking that maybe it's possible / efficient to only produce the outer_edge and vert_masks if those sockets are connected.

I think it will not be efficient. I don't use any preparation for these sockets, just assign values for variables in different places of the code. I think that checking boolean values only can to increase time of runtime.

at a later stage we can discuss making it possible to set a unique shift value for every original vertex

I tried to imagine situation when it can be useful. I suppose that in most cases edges_in will be unordered so it will be difficult to prepare data for offset socket and receive interesting result. May with curve in node it could be interesting to have such feature but this node haven't done yet as i know : P. At this moment I prepare offset socket only for different values per object.

TWO_PI = 2 * pi

I am not quite understand benefit of creating the variable. Why not just to use 2 * pi into my function?

@zeffii
Copy link
Collaborator

zeffii commented Dec 21, 2017

because calc_angle is a function you call many many times, and instead of recalculating 2*pi on each call, you can simply reference the answer (TWO_PI). It's a little bit more efficient. It's also standard graphics programming practice.

@Durman
Copy link
Collaborator Author

Durman commented Dec 22, 2017

I added this node in Alpha. Must it be in beta?

@zeffii
Copy link
Collaborator

zeffii commented Dec 22, 2017

No, alpha is fine )

@Durman
Copy link
Collaborator Author

Durman commented Dec 22, 2017

It is seems that I fixed all. ))

@enzyme69
Copy link
Collaborator

enzyme69 commented Dec 22, 2017 via email

@Durman
Copy link
Collaborator Author

Durman commented Dec 22, 2017

@zeffii

Why 2 calc_angle functions, keep the outermost that uses TWO_PI

Sorry, don't understand.

@Durman
Copy link
Collaborator Author

Durman commented Dec 22, 2017

@zeffii Yes, didn't notice :/

@zeffii
Copy link
Collaborator

zeffii commented Dec 22, 2017

i have a question about the verts mask output.

  • if you never rearrange the input verts and don't remove any
  • then the final verts_mask is a single step
def make_verts_new_mask(verts_in, verts_out):
    num_original_verts = len(verts_in)
    num_verts_out = len(verts_out)
    num_new_verts = num_verts_out - num_original_verts
    f = ([0] * num_original_verts) + ([1] * num_new_verts)
    return f
    

f = make_verts_new_mask(verts_in=[5,2,3,4,5], verts_out=[5,2,3,4,5,5,5,5,2,2,2,3,3,3,4])
print(f)  # [0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]

@Durman
Copy link
Collaborator Author

Durman commented Dec 22, 2017

@zeffii

i have a question about the verts mask output.

It's not quite exactly. In my script at first I add new points and after that add old points thus the order is changed.
I have only three strings that is responsible for this.

vers_mask = [0 for _ in verts_out]
...
      vers_mask.append(1)
...
      vers_mask.append(1)

Is there point to change this? I think it must work faster than if writ seperate function for this.

@zeffii
Copy link
Collaborator

zeffii commented Dec 22, 2017

OK .

It's not quite exactly. In my script at first I add new points and after that add old points thus the order is changed.

this is not a problem. I will merge if you think you're ready to have more people test it :)

@Durman
Copy link
Collaborator Author

Durman commented Dec 23, 2017

Ok. I am ready. ))

@zeffii zeffii merged commit 81df95c into nortikin:master Dec 23, 2017
@zeffii
Copy link
Collaborator

zeffii commented Dec 23, 2017

thanks for the cool new node @Durman .

hopefully the process was not too painful. First time is the most difficult (eye - opening).

@zeffii
Copy link
Collaborator

zeffii commented Dec 24, 2017

i think i can implement variable offset, if you're interested.

@zeffii
Copy link
Collaborator

zeffii commented Dec 24, 2017

imagine for a moment that we can generate a list of offset radii with the same length as verts_in, (we have a function for this already in utils/core "match_long_repeat") Then rather than shift being static per mesh, it can be variable.

   #Setting points
   findex_new_points = [0]
   for i in range(len(verts_in)):
        if len(neighbours[i]) == 1:
            verts_out.extend(get_end_points(i))
            findex_new_points.append(findex_new_points[-1] + 2)
        else:
            p = get_middle_points(i)
            verts_out.extend(p)
            findex_new_points.append(findex_new_points[-1] + len(p))

and..

   # (not displayed here) we always ensure radii is as long as verts_in
   # meaning we can enumerate radii and use the idx it produces

   findex_new_points = [0]
   for idx, radius in enumerate(radii):
        if len(neighbours[idx]) == 1:
            verts_out.extend(get_end_points(idx, radius))
            findex_new_points.append(findex_new_points[-1] + 2)
        else:
            p = get_middle_points(idx, radius)
            verts_out.extend(p)
            findex_new_points.append(findex_new_points[-1] + len(p))

for speed I would probably make variable offset optional. and modify the offset_edge code to pick which of the proposed loops to use depending on what's needed. Maybe it will make more sense if I just do it.

@Durman
Copy link
Collaborator Author

Durman commented Dec 24, 2017

@zeffii Okay, I agree with it.
I also can do this, and also to do experiments with Z coordinate. Not sure that the node have to have constraints with Z values and to exist only in XY surface. It is easy to add Z coordinate but difficult to calculate normals correctly.

@zeffii
Copy link
Collaborator

zeffii commented Dec 24, 2017

Z stuff is not important / not interesting use of this node. @enzyme69 often tries to cut grass using a nail clipper. (wrong tool for job)

@enzyme69
Copy link
Collaborator

enzyme69 commented Dec 25, 2017 via email

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.

3 participants