-
Notifications
You must be signed in to change notification settings - Fork 350
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
Refactor idGameEdit #369
base: master
Are you sure you want to change the base?
Refactor idGameEdit #369
Conversation
Sorry, somehow forgot about this. TBH, if this doesn't really fix the UBSAN problems (as the game lib still can't be built) I don't really see the point of this change. Implementing static linking of the game lib seems like the way to go, and shouldn't be too hard (see d3wasm) - though in case of dhewm3 it of course needs to be optional (enabled by a CMake option that's off by default - ideally we could switch between "off" / "statically link base" / " statically link d3xp"). |
Well there's also the FIXME comment (that I seem to have forgotten to remove) that says this should be done. @HarrievG do you want to incorporate this into your changes? |
Please don't - this breaks the Game API (=> makes mod DLLs incompatible), without solving any problem (that old FIXME isn't worth it) If we break the game API in the future for some other reason we could think about incorporating this change |
I was under the impression that he needed to change this class anyway. |
As far as I see #339 doesn't touch Game.h |
I'll check it out later this night or this week, will rebase, and make it
ready.
…On Thu, Apr 22, 2021, 15:54 Daniel Gibson ***@***.***> wrote:
As far as I see #399 doesn't touch Game.h
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#369 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIUHNXFZWUJL2HTQYOLARDTKATBBANCNFSM43BDSBYA>
.
|
"As far as I see #339 doesn't touch Game.h" "If we break the game API in the future for some other reason we could think about incorporating this change" |
Well, we are waiting :D |
Rebased and removed the fixed FIXME comment. |
virtual void ParseSpawnArgsToRefSound( const idDict *args, refSound_t *refSound ) = 0; | ||
|
||
// Animation system calls for non-game based skeletal rendering. | ||
virtual idRenderModel * ANIM_GetModelFromEntityDef( const char *classname ) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect const
getters.
|
||
// In game map editing support. | ||
virtual const idDict * MapGetEntityDict( const char *name ) const = 0; | ||
virtual void MapSave( const char *path = NULL ) const = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚑
Virtual functions bind dynamically, but default function parameter values bind statically.
So if you implement this function in a derived class with a different default parameter, and you call it through a base pointer without parameter, it will use the default parameter specified in the base class, not the one in the derived class!
virtual void TriggerSelected() = 0; | ||
|
||
// Entity defs and spawning. | ||
virtual const idDict * FindEntityDefDict( const char *name, bool makeDefault = true ) const = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment on line 311.
virtual void EntitySetModel( idEntity *ent, const char *val ) = 0; | ||
virtual void EntityStopSound( idEntity *ent ) = 0; | ||
virtual void EntityDelete( idEntity *ent ) = 0; | ||
virtual void EntitySetColor( idEntity *ent, const idVec3 color ) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second parameter is copied.
Split
idGameEdit
into base classidGameEditBase
which is in the main binary andidGameEdit
which is only referenced in the game libraries.This was an attempt to fix UBSAN compilation. The main binary now compiles with UBSAN but the game library does not. It requires vtables of things which only exist in the main binary. I think the way to make it work will be to implement static linking of game library. So this might not even be necessary. Thoughts?