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 unijson parser #184

Merged
merged 4 commits into from
Feb 27, 2019
Merged

Use unijson parser #184

merged 4 commits into from
Feb 27, 2019

Conversation

ousttrue
Copy link
Contributor

@ousttrue ousttrue commented Feb 6, 2019

Add experimental switch ImporterContext.UseUniJSONParser .

@ousttrue ousttrue added this to the v0.51 milestone Feb 6, 2019
@ousttrue ousttrue changed the title Use unijson parser WIP Use unijson parser Feb 6, 2019
@ousttrue ousttrue changed the title WIP Use unijson parser Use unijson parser Feb 6, 2019
@ousttrue ousttrue modified the milestones: v0.51, v0.50 Feb 8, 2019
@yutopp yutopp modified the milestones: v0.50, v0.51.0 Feb 12, 2019
@ousttrue ousttrue changed the title Use unijson parser WIP Use unijson parser Feb 12, 2019
@ousttrue ousttrue changed the title WIP Use unijson parser Use unijson parser Feb 12, 2019
@ousttrue
Copy link
Contributor Author

Separate #189

@ousttrue ousttrue modified the milestones: v0.51.0, v0.52.0 Feb 13, 2019
Copy link
Contributor

@yutopp yutopp left a comment

Choose a reason for hiding this comment

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

LGTM with nits

{

}

public glTFBuffer(IBytesBuffer storage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public glTFBuffer(IBytesBuffer storage)
public glTFBuffer(IBytesBuffer storage) : this()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that it is not necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, sorry. Other fields are not initialized by the default constructor. You are right.

public glTFMesh()
{
}

public glTFMesh(string _name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public glTFMesh(string _name)
public glTFMesh(string _name) : this()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that it is not necessary

@yutopp
Copy link
Contributor

yutopp commented Feb 14, 2019

Please wait merging this PR until v0.51.0 is released 🙏

@ousttrue ousttrue merged commit 5654604 into master Feb 27, 2019
@ousttrue ousttrue deleted the use_unijson_parser branch February 27, 2019 18:15
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