Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Refactors to the code #1467

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

Closed
Pirulax wants to merge 41 commits into multitheftauto:master from Pirulax:refactor/asMuchAsICan

Conversation

@Pirulax
Copy link
Contributor

@Pirulax Pirulax commented May 26, 2020

Alright, here I'll try to refactor a lot of code, while in the same time having meaningful commits.
Im creating this PR, so the changes can be reviewed if needed.
Also, Im trying not to refactor the code too much, so I dont introduce bugs, because that woudnt be fun.

Mostly I refactor loop to use 'range based for loop' I also add a lot of 'auto', and 'constexpr' where needed.

Pirulax added 21 commits May 26, 2020 01:58
Copy link
Contributor Author

Pirulax commented May 26, 2020

Yeah so I've already fucked up my clean commit list... 😁😁

Comment on lines 488 to +495
{
const bool bDrawModeChanging = (m_CurDrawMode != newDrawMode);
const bool bBlendModeChanging = (m_CurBlendMode != newBlendMode);

// Flush old
if (m_CurDrawMode == EDrawMode::DX_SPRITE)
{
m_pDevice->SetSamplerState(0, D3DSAMP_ADDRESSU, D3DTADDRESS_WRAP);
m_pDevice->SetSamplerState(0, D3DSAMP_ADDRESSV, D3DTADDRESS_WRAP);
m_pDXSprite->End();
}
else if (m_CurDrawMode == EDrawMode::DX_LINE)
{
m_pLineInterface->End();
}
else if (m_CurDrawMode == EDrawMode::TILE_BATCHER)
{
m_pTileBatcher->Flush();
}
else if (m_CurDrawMode == EDrawMode::PRIMITIVE)
{
m_pPrimitiveBatcher->Flush();
}
else if (m_CurDrawMode == EDrawMode::PRIMITIVE_MATERIAL)
{
m_pPrimitiveMaterialBatcher->Flush();
}
// Draw mode changing?
if (!bDrawModeChanging && !bBlendModeChanging)
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the extra scope here for? Also there's not really a need for const here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, just wanted to indicate neither of the 2 bools are used later. Putting the in a scope indicates that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved?

Pirulax added 12 commits May 29, 2020 04:58
Copy link
Contributor

qaisjp commented May 29, 2020 •
edited
Loading

This PR is (削除) getting (削除ここまで) too big + disorganised.

Copy link
Contributor Author

Pirulax commented May 31, 2020

I agree. Ill open separate PRs.

Comment on lines -85 to 93

CVector2D operator+(const CVector2D& vecRight) const { return CVector2D(fX + vecRight.fX, fY + vecRight.fY); }
constexprCVector2D operator+(const CVector2D& vecRight) constnoexcept { return CVector2D(fX + vecRight.fX, fY + vecRight.fY); }

CVector2D operator-(const CVector2D& vecRight) const { return CVector2D(fX - vecRight.fX, fY - vecRight.fY); }
constexprCVector2D operator-(const CVector2D& vecRight) constnoexcept { return CVector2D(fX - vecRight.fX, fY - vecRight.fY); }

CVector2D operator*(const CVector2D& vecRight) const { return CVector2D(fX * vecRight.fX, fY * vecRight.fY); }
constexprCVector2D operator*(const CVector2D& vecRight) constnoexcept { return CVector2D(fX * vecRight.fX, fY * vecRight.fY); }

CVector2D operator/(const CVector2D& vecRight) const { return CVector2D(fX / vecRight.fX, fY / vecRight.fY); }
constexprCVector2D operator/(const CVector2D& vecRight) constnoexcept { return CVector2D(fX / vecRight.fX, fY / vecRight.fY); }

void operator+=(float fRight)
constexprvoid operator+=(float fRight)noexcept
{
fX += fRight;
fY += fRight;
Copy link

Choose a reason for hiding this comment

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

I don't think noexcept is needed here. The CVector classes don't throw any exceptions. Also, I feel like constexpr is not useful here since the result is usually not known at compile time. The compiler is smart enough to know when to inline a function. This is just making the code hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please comment to #1478, and lets discuss it there.
tbh I agree, but nobody reads CVector code anyways. Constexpr is needed(at least for the constructor) in CGraphics refactor(somewhere in the end, the sphere thing)

Copy link
Contributor Author

Pirulax commented Jun 18, 2020

As noted above, this is getting disorganized. I'll close it, and do separate PRs for the ease of testing, and merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ccw808 ccw808 ccw808 left review comments

@Lpsd Lpsd Lpsd left review comments

+2 more reviewers

@qaisjp qaisjp qaisjp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /