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

SMPL data converter #151

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

WYK96
Copy link
Contributor

@WYK96 WYK96 commented Jan 15, 2024

Add SMPL data converter that:

  1. convert the AMASS data or Humandata into the smpl data.
  2. evaluate the structure of the stream data.

Comment on lines +207 to +211
data_type = self.data_converter.get_data_type(file_path)
# organize the input data as the smpl data
if data_type is SMPLDataTypeEnum.AMASS:
self.logger.info('Received AMASS data, converting to SMPL(X) data')
data = self.data_converter.from_amass(file_path)
Copy link
Member

@LazyBusyYang LazyBusyYang Jan 15, 2024

Choose a reason for hiding this comment

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

Too many file IO, both get_data_type and from_amass load file from file_path. Something like this will be faster:

...
def get_data_type(self, file: Union[str, io.BytesIO]) -> str:
...
with open(file_path, 'rb') as f:
    data_type = self.data_converter.get_data_type(file_path)
    ....
    smpl_data = self.data_converter.from_amass(file_path)

"""
self.logger = logger

def get_data_type(self, filepath: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

get_data_type, from_humandata and from_amass looks more like static methods. Try static class and add this

def __new__(cls):
        raise NotImplementedError("SMPLDataConverter cannot be instantiated")

if gender is None:
gender = 'neutral'
self.logger.warning(
f'Cannot find gender record in {human_data}.meta, ' +
Copy link
Member

Choose a reason for hiding this comment

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

I doubt the readability of this log. Can you provide an example from an actual usage where a warning is raised here?

return SMPLDataTypeEnum.UNKNOWN

def from_humandata(self,
filepath: str) -> Optional[Union[SMPLData, SMPLXData]]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filepath: str) -> Optional[Union[SMPLData, SMPLXData]]:
filepath: str) -> Optional[Union[SMPLData, SMPLXData, None]]:


res = SMPLData(gender=gender, logger=self.logger)
res.from_param_dict(param_dict)
mask = np.ones((n_frames, ), dtype=np.uint8)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mask = np.ones((n_frames, ), dtype=np.uint8)

return res

def from_amass(self,
filepath: str) -> Optional[Union[SMPLData, SMPLXData]]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filepath: str) -> Optional[Union[SMPLData, SMPLXData]]:
filepath: str) -> Optional[Union[SMPLData, SMPLXData, None]]:

data.dump(file_path)
elif data_type is SMPLDataTypeEnum.HUMANDATA:
self.logger.info('Received HumanData, converting to SMPL(X) data')
data = self.data_converter.from_humandata(file_path)
Copy link
Member

Choose a reason for hiding this comment

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

Easier to find out what type the return value is.

Suggested change
data = self.data_converter.from_humandata(file_path)
smpl_data = self.data_converter.from_humandata(file_path)

What if self.data_converter returns None?

return True


class SMPLDataConverter:
Copy link
Member

Choose a reason for hiding this comment

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

We've already got data converters for dataset, shall we have a different name here?

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