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

4.0 #802

Closed
wants to merge 2 commits into from
Closed

4.0 #802

wants to merge 2 commits into from

Conversation

jvbsl
Copy link
Contributor

@jvbsl jvbsl commented Aug 21, 2018

Unfortunately we were working in parallel on some stuff and had a lot of merge conflicts. But I tried to use the solutions which in my opinon seemed to be the nicer solution.

Purpose of this PR

  • Fixed a lot of stylecop errors(.OpenAL and .Mathematics)
  • reinstalled stylecops for .Mathematics and .OpenAL(still compiles through fixes)
  • refactored EaxReverb and EfxEaxReverb
  • fixed some XML comments(warnings)

Testing status

Well it does build and the only implementation I changed isn't in use anywhere...

Comments

EaxReverb and EfxEaxReverb need to be reviewed. But beforehand it was a total mess an a mix of DirectSound like Reverb(even mixing between two definitions of DS) and the old OpenAL. I Changed it to use the deprecated OpenAL Standard only, while converting all the Presets to EfxEaxReverb(because EaxReverb alone does not support all necessary properties - at least without DirectSound properties mixed in).
Im pretty sure the change I did was correct and necessary. But please confirm(and I hope it does not break anything - could be that it does break old projects though, but I think better refactor it now than later?)

* stylecop.props import readded to .Math and fixed all stylecop errors

* fixed some xml comment inconsistencies

* fixed a few .OpenAL stylecop errors
* fixed stylecop errors

* fixed some XML comment warnings

* refactored EfxEaxReverb and EaxReverb
@Nihlus
Copy link
Contributor

Nihlus commented Aug 22, 2018

Thanks!

A lot of this work is still duplicated in other PRs - at the moment, you might want to avoid Generator.Bind (due to #795). You're also not following a lot of the conventions the library uses.

See #804 for a PR with the style guide that's being discussed - I should have put it up earlier, but some personal stuff got in the way.

As for OpenAL, I have a PR coming in for that portion of the library soon which already fixes the mentioned issues.

Keep the changes to Mathematics, though! I'll review those bits as soon as you've brought the rest of the PR up to snuff.

@jvbsl
Copy link
Contributor Author

jvbsl commented Aug 22, 2018

So is something like that:
0e047af#diff-a1b1ddf2ad3dd595a4c56bfbad3d6ce2R90
actually OK, or not? Cause it does breach rule 2.28 but in my opinion the reason of better readability by having it aligned in matrix form would be a good one?

Sorting of members in files shouldn't have been changed by me, but I can still do that. Apart from that I couldn't see anything by scrolling through just now. Could you perhaps name the types of violations I need to specifically look out for?(So that it's easier for me to find them)

At least it does not seem to be violations which are detected by StyleCop(yet?).

Thanks would highly appreciate it ;)

@Nihlus
Copy link
Contributor

Nihlus commented Aug 23, 2018

That should stay as-is, yeah. I can do a full review once you've removed the changes to OpenAL and Bind :)

@jvbsl
Copy link
Contributor Author

jvbsl commented Aug 23, 2018

Should I redo the complete thing and use force push, to keep the history clean? Because reverting so much code back with new commits seems to me to get an ugly commit history. And as nobody should use my fork I think it is not as bad to use force push compared to the history.
But I don't want to go against any rules in that aspect^^

@FreezyLemon
Copy link
Contributor

Maybe you should even close this one, and open a new PR on another branch (it's not that big of a deal, but as a heads up, your branch should have a different name from the one you're trying to merge your changes into).
Plus if you do keep this PR you'll have to rewrite the entire description and force-push all the existing commits "away" anyways.

up to you in the end though ¯\(ツ)

@jvbsl
Copy link
Contributor Author

jvbsl commented Aug 23, 2018

Normally I would have done it on a different branch but as all that simple refactoring seems to get commited directly onto that branch I thought that I would do the same, but I can change that.

I have already the changes(even changed some things again) locally.

I don't think it's completely up to me, because I want my PR to be accepted ;)

@Nihlus
Copy link
Contributor

Nihlus commented Aug 23, 2018

As a general rule, we use feature branches for every pull request. Force pushing is fine.

@jvbsl
Copy link
Contributor Author

jvbsl commented Aug 23, 2018

#806 should be the new and corrected one in a separate feature-branch

@jvbsl jvbsl closed this Aug 23, 2018
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

3 participants