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

Use abc.ABCMeta for Geometry class #292

Merged
merged 4 commits into from
Jan 14, 2019
Merged

Conversation

wkentaro
Copy link
Contributor

To avoid expected but not implemented methods like apply_translation of
PointCloud.

To avoid expected but not implemented methods like apply_translation of
PointCloud.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 83.534% when pulling 3d30ce1 on wkentaro:use_abc_geometry into ed936fc on mikedh:master.

@coveralls
Copy link

coveralls commented Jan 13, 2019

Coverage Status

Coverage decreased (-0.07%) to 83.887% when pulling 29afcb3 on wkentaro:use_abc_geometry into ed936fc on mikedh:master.

@mikedh
Copy link
Owner

mikedh commented Jan 13, 2019

Great idea!

I might add a comment in the top of the Geometry object explaining why we're using a metaclass:

The `Geometry` object is the parent object of geometry classes, including:
Trimesh, PointCloud, and Scene objects.

By decorating a method with `abc.abstractmethod` it just means the objects that 
inherit from `Geometry` MUST implement those methods. 

One other thought: apply_translation, apply_scale can actually be implemented in the Geometry object as they just generate a transform and are the same everywhere, they just call self.apply_transform that needs the per- object implementation (it could be the abc.abstractmethod)

It looks like it needs some tweaks to pass in Python 2, there's a discussion here, it looks like the cleanest solution is (IMHO):

import sys
import abc

if sys.version_info >= (3, 4):
    ABC = abc.ABC
else:
    ABC = abc.ABCMeta('ABC', (), {})

class SomeAbstractClass(ABC):
    @abc.abstractmethod
    def do_something(self):
        pass

@mmatl
Copy link
Contributor

mmatl commented Jan 14, 2019

@mikedh Another option is using six, although it adds another (very light) dependency for Python 2/3 interop. Then, it would just look like:

import abc
import six

@six.add_metaclass(abc.ABCMeta)
class SomeAbstractClass(object):
    @abc.abstractmethod
    def do_something(self):
        pass

I think that should work.

@wkentaro
Copy link
Contributor Author

wkentaro commented Jan 14, 2019

I updated. I avoided to use six module since we only need that module only for handling ABC class.
We may use six if we also have another case we need it.

@mikedh
Copy link
Owner

mikedh commented Jan 14, 2019

Looks good thanks! Yeah including six at some point may be the way to go if we run in to something particularly annoying. It's MIT licensed single- file module so it won't mess with the minimal install if we just include it as trimesh/six.py. That being said it hasn't been necessary yet, and this fix looks good.

@mikedh mikedh merged commit 498da15 into mikedh:master Jan 14, 2019
mikedh added a commit that referenced this pull request Jan 14, 2019
@wkentaro wkentaro deleted the use_abc_geometry branch January 14, 2019 16:36
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.

None yet

4 participants