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

Effekseer #3283

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

TheNormalnij
Copy link
Contributor

@TheNormalnij TheNormalnij commented Jan 5, 2024

Effekseer is a particle effect editing tool. It can be added to have rich effects in MTA. Effekseer includes material editor and effect editor.
Demo with free samples: https://youtu.be/Qk2xIfkzEfw

Pros:

Cons:

  • this feature can be too big for MTA
  • tools is not trivial for use
  • some effects might not work in DirectX 9
  • changes MTA look

Current state:

  • polish codebase
  • fix edge cases
  • materials don't use some input values (time)

Notes:

  • Materials work only with cache. Cache can be created in Effekseer editor

void CCore::DestroyEffekseer()
{
WriteDebugEvent("CCore::DestroyEffekseer");
if (m_pEffekseer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this condition needed?


private:
CClientManager* m_pManager;
std::list<CClientEffekseerEffect*> m_List;
Copy link
Collaborator

Choose a reason for hiding this comment

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

std::vector would be more efficient here. You can use swap-and-pop idiom to remove elements with O(1).

Client/mods/deathmatch/logic/CClientEffekseerEffect.h Outdated Show resolved Hide resolved

void CEffekseerManager::Remove(CEffekseerEffect* effect)
{
delete static_cast<CEffekseerEffectImpl*>(effect);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it better to make a destructor virtual and avoid the cast?

Client/mods/deathmatch/logic/CClientEffekseerEffect.h Outdated Show resolved Hide resolved

CClientEffekseerEffectHandler* CClientEffekseerEffect::Play(const CVector& pos)
{
auto handle = m_pEffect->Play(pos);
Copy link
Collaborator

Choose a reason for hiding this comment

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

handle can be potentially invalid

SetTypeName("effekseer-effect");

m_pManager = pManager;
m_pEffect = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

m_pEffect can be nulled in the member initializer list(or even better as a default member value).

// Effect will be removed here
m_pInterface->StopEffect(m_Handle);
// Free handler too
delete this;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we avoid this self destruction?

Client/core/CCore.cpp Outdated Show resolved Hide resolved
@TheNormalnij
Copy link
Contributor Author

This is only concept @tederis . PR isn't ready for review. Сode quality is very low at this point and things don't work correctly.
Anyway. Thanks you for some suggestions,

@tederis
Copy link
Collaborator

tederis commented Jan 6, 2024

This is only concept @tederis . PR isn't ready for review. Сode quality is very low at this point and things don't work correctly. Anyway. Thanks you for some suggestions,

Oh, those are just comments. I'm just giving you insights(I hope).

@@ -71,3 +76,68 @@ float CEffekseerEffectHandlerImpl::GetDynamicInput(int32_t index)
{
return m_pInterface->GetDynamicInput(m_Handle, index);
}

void CEffekseerEffectHandlerImpl::SetRandomSeed(int32_t seed)
Copy link
Contributor

@Pirulax Pirulax Jan 8, 2024

Choose a reason for hiding this comment

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

Is there a reason for creating a whole interface over effekseer?
I guess it's so than effekseer can be a dll, but I don't think there's any benefit from 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.

Is there a reason for creating a whole interface over effekseer?

This is an adapter between MTA types and Effekseer types.

  • Main integration part is under one project.
  • From an architecture perspective that makes future vendor updates much easier.
  • Effekseer namespaces brings an additional mess in MTA projects.
  • Dependency on interfaces is better than on implementation.

I guess it's so than effekseer can be a dll, but I don't think there's any benefit from that.

I use dynamic linking for Effekseer over static because i need that Interface in both Client Core and Client Deathmatch projects. I can be heavily wrong, but that cause compiled code duplication in this projects with static linking. I saw this two years ago, when i was truing to compile Effekseer first time. I didn't remember all details.

I would prefer to link Effekseer with Client core project, when i can. I don't know if this is possible yet.

@tederis tederis added the enhancement New feature or request label Feb 18, 2024
@github-actions github-actions bot added the stale Inactive for over 90 days, to be closed label May 19, 2024
Copy link
Contributor

This draft pull request is stale because it has been open for at least 90 days with no activity. Please continue on your draft pull request or it will be closed in 30 days automatically.

@TracerDS
Copy link
Contributor

TracerDS commented Jun 8, 2024

#3437 related

Any update on this?

@github-actions github-actions bot removed the stale Inactive for over 90 days, to be closed label Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants