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

Move MaxUboSize definition #530

Merged
merged 3 commits into from Dec 18, 2018
Merged

Move MaxUboSize definition #530

merged 3 commits into from Dec 18, 2018

Conversation

marysaka
Copy link
Contributor

@marysaka marysaka commented Dec 7, 2018

This fix a crash on Ryujinx.ShaderTools caused by the absence of an
OpenGL context.

This fix a crash on Ryujinx.ShaderTools caused by the absence of an
OpenGL context.
@marysaka marysaka added the gpu Related to Ryujinx.Graphics label Dec 7, 2018
@@ -106,6 +108,8 @@ public GlslDecompiler()
{ ShaderIrInst.Utof, GetUtofExpr },
{ ShaderIrInst.Xor, GetXorExpr }
};

GlslDecompiler.MaxUboSize = MaxUboSize / 16;
Copy link
Member

Choose a reason for hiding this comment

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

IMO, its weird that a static member is being set on the constructor. I think its better to either make it a instance member, or allow it to be set externally aswell.

Copy link
Contributor

Choose a reason for hiding this comment

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

Imo, a readonly member would be the best approach to this, as unnecessarily introducing statics is a bad way to go about it. Because what if we somehow need to have 2 instances of GlslDecl with a different MaxUboSize (not sure how likely this will be but hypothetically speaking)

Copy link
Member

@gdkchan gdkchan left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@gdkchan gdkchan merged commit 33e7c89 into Ryujinx:master Dec 18, 2018
@marysaka marysaka deleted the shader-tools-fixes branch January 23, 2019 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gpu Related to Ryujinx.Graphics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants