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

EXTENSION: Add MVD_PEXT1_EXTRA_PFS. #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsvensson
Copy link

mvdsv and ezQuake had a broken implementation of FTE_PEXT_TRANS since inception. To fix this, future versions of ezQuake will enable this extension in which case mvdsv will write extra playerflags.

To accomodate for these bits PF_ONGROUND and PF_SOLID will be moved if the extra byte is written. This is how FTE client and server works.

@tcsabina
Copy link
Collaborator

Nano,

Should we not increase PROTOCOL_VERSION?
V28 is the 'one' that is currently used for everything. I think we need a version increment to clearly indicate that there is a change.

Additionally, we need github action script that builds exquake, mvdsv with a new version from a PR. Willing to contribute? ;)

@dsvensson
Copy link
Author

If it's the norm to update PROTOCOL_VERSION every time an optional protocol extension is introduced, then yes.

mvdsv and ezQuake had a broken implementation of FTE_PEXT_TRANS since
inception. To fix this, future versions of ezQuake will enable this
extension in which case mvdsv will write extra playerflags.

To accomodate for these bits PF_ONGROUND and PF_SOLID will be moved if
the extra byte is written. This is how FTE client and server works.
#define PF_ONGROUND (1<<14) // ZQuake extension
#define PF_SOLID (1<<15) // ZQuake extension
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add as comment the closing definition:
#endif // FTE_PEXT_TRANS

@@ -291,8 +293,13 @@ Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
// bits 11..13 are player move type bits (ZQuake extension)
#define PF_PMC_SHIFT 11
#define PF_PMC_MASK 7
#ifdef FTE_PEXT_TRANS
#define PF_ONGROUND (1<<22) // ZQuake extension, 14 offset to extra playerflags
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to use indentation to indicate the parent definition.
These files are containing nothing but ifdefs and defines (more or less). Using indentation makes it easier to read.

#ifdef A
# define B
#else
# define C
# ifdef D
#  define E
# endif // D
#endif //A

Copy link
Author

@dsvensson dsvensson Jan 31, 2023

Choose a reason for hiding this comment

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

@tcsabina as you see here, FTE_PEXT_TRANS will be defined, and then the PF_ONGROUND and PF_SOLID would be offsetted, and the code in ezquake would have to deal with that as it should have from day one when it adopted FTE_PEXT_TRANS. So if this was merged, and ezquake got the new protocol.h file without updated cl_ents.c it would not be able to connect to any server. So this is an API change between <project ezquake> and <project protocol.h> unrelated to protocol changes.

Copy link
Author

@dsvensson dsvensson Jan 31, 2023

Choose a reason for hiding this comment

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

Good thing is that this part of the alpha support is a trivial change in ezquake once merged in qwprot. Can be dealt with separately from the graphical updates. Could actually be merged before via ifdef's now that I think of it.

@tcsabina
Copy link
Collaborator

If it's the norm to update PROTOCOL_VERSION every time an optional protocol extension is introduced, then yes.

I am not sure, to be honest. Not sure how the other projects check the protocol version for enable/disable certain features, or just checking if a define is there.

But in this particular case, which is a little extension as you've mentioned, maybe we can stick with the same version number.

@tcsabina
Copy link
Collaborator

nano,

I will close this PR, just to reopen in a minute, to trigger the new pipelines.

@tcsabina tcsabina closed this Jan 31, 2023
@tcsabina tcsabina reopened this Jan 31, 2023
@dsvensson
Copy link
Author

Should allow ezq to compile, but ezq would be broken as it also needs to adapt to the changes. Otherwise it would use the changed defines incorrectly. I'll have a look later.

@tcsabina tcsabina closed this Jan 31, 2023
@tcsabina
Copy link
Collaborator

tcsabina commented Jan 31, 2023

, but ezq would be broken as it also needs to adapt to the changes. Otherwise it would use the changed defines incorrectly. I'll have a look later.

You mean that although ezquake will be compiled, it will still be broken. Correct?

@tcsabina tcsabina reopened this Jan 31, 2023
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

2 participants