search

readyplayerme / rpm-unreal-sdk

Ready Player Me Unreal SDK
Other
23 stars 9 forks source link

SDK-263 feat: replaces meshLod property with lod #21

Closed MaxAndreassenRPM closed 1 year ago

MaxAndreassenRPM commented 1 year ago

SDK-263

me.atlassian.net/jira/software/c/projects/SDK/boards/6?modal=detail&selectedIssue=SDK-263)

Description

Replace meshLod property with lod property when requesting from the Ready Player Me API.

gadamyan commented 1 year ago

The PR looks good since the change is the same as in Unity. I have a question to @rYuuk . Is it a good idea not to change the param name in the avatar config, and not to introduce a breaking change? I'm asking because it's a bit confusing that the param name in the avatar config is MeshLod but the string that we send it Lod.

rYuuk commented 1 year ago

The PR looks good since the change is the same as in Unity.

I have a question to @rYuuk . Is it a good idea not to change the param name in the avatar config, and not to introduce a breaking change?

I'm asking because it's a bit confusing that the param name in the avatar config is MeshLod but the string that we send it Lod.

We did the same in unity https://github.com/readyplayerme/rpm-unity-sdk-core/pull/90 I think it make sense to keep the name in avatar config as MeshLod even though internal parameter is lod, that way if it's changed again in future we will only change the internal parameter.

HarrisonHough commented 1 year ago

The PR looks good since the change is the same as in Unity. I have a question to @rYuuk . Is it a good idea not to change the param name in the avatar config, and not to introduce a breaking change? I'm asking because it's a bit confusing that the param name in our SDK's avatar config is MeshLod but the string that we send it Lod.

We did the same in unity readyplayerme/rpm-unity-sdk-core#90 I think it make sense to keep the name in avatar config as MeshLod even though internal parameter is lod, that way if it's changed again in future we will only change the internal parameter.

totally agree with this. To me it is actually dumb that they decided to rename param as lod on backend. Perhaps it was done to separate it from the old meshLod but "meshLod" as a name is much better as it more accurately reflects what it does, it only effects the mesh quality, nothing else. In any case it's good that users will see it as MeshLod in avatar config as it is clear what it effects.