Skip to content

  • Projects
  • Groups
  • Snippets
  • Help
  • This project
    • Loading...
  • Sign in / Register
SRB2
SRB2
  • Project
    • Overview
    • Details
    • Activity
    • Cycle Analytics
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Charts
  • Issues 246
    • Issues 246
    • List
    • Board
    • Labels
    • Milestones
  • Merge Requests 71
    • Merge Requests 71
  • Wiki
    • Wiki
  • Members
    • Members
  • Collapse sidebar
  • Activity
  • Graph
  • Charts
  • Create a new issue
  • Commits
  • Issue Boards
  • STJr
  • SRB2SRB2
  • Merge Requests
  • !1010

Merged
Opened Jun 20, 2020 by James R.@james 
  • Report abuse
Report abuse

Use ordered id for netvars instead of shitty hash

This superscedes !980 (closed) and addresses concerns over demo compatibility.

  • Discussion 12
  • Commits 7
  • Changes 5
{{ resolvedDiscussionCount }}/{{ discussionCount }} {{ resolvedCountText }} resolved
  • toaster @toaster commented Jul 04, 2020
    Master

    Hold on, this doesn't address my concern regarding demo compatibility. It maintains compatibility with demos that have already been created, but writes new demos that will have the same issue outlined in my comment on the previous merge request.

    Quoted for comparison:

    I do not believe it is a solid approach. Specifically, it prevents replay compatibility between minor versions that add or remove cvars, whether that's versions of Lua mods or the base game in the case of Kart's netreplays, in a way that silently fails càstrophically.

    You have addressed compatibility with old versions, but you have not addressed compatibility between subsequent patches/mods that introduce additional netvars.

    Edited Jul 04, 2020 by toaster
    ~~Hold on, this doesn't address my concern regarding demo compatibility. It maintains compatibility with demos that have already been created, but writes new demos that will have the same issue outlined in my comment on the previous merge request.~~ ~~Quoted for comparison:~~ > I do not believe it is a solid approach. Specifically, it prevents replay compatibility between minor versions that add or remove cvars, whether that's versions of Lua mods or the base game in the case of Kart's netreplays, in a way that silently fails càstrophically. > ~~You have addressed compatibility with old versions, but you have not addressed compatibility between subsequent patches/mods that introduce additional netvars.~~
  • LJ Sonic @LJSonic commented Jul 04, 2020
    Master

    New demo files should use plain cvar names, if it's not the case already.

    New demo files should use plain cvar names, if it's not the case already.
  • toaster @toaster commented Jul 04, 2020
    Master

    ...somehow I misread the diff initially, and was just about to place a new comment when you explained it yourself. Thanks for pointing it out; however, I'm confident I've read the diff properly this time and although I see where the name is read from demos, I don't see any place where it is written to them. As of 2.2.0 it used CV_SaveNetVars(), which writes the netid regardless of context, and I don't see that replaced anywhere in this diff. How is that handled?

    Edited Jul 04, 2020 by toaster
    ...somehow I misread the diff initially, and was just about to place a new comment when you explained it yourself. Thanks for pointing it out; however, I'm confident I've read the diff properly this time and although I see where the name is read from demos, I don't see any place where it is *written* to them. As of 2.2.0 it used `CV_SaveNetVars()`, which writes the netid regardless of context, and I don't see that replaced anywhere in this diff. How is that handled?
  • LJ Sonic @LJSonic commented Jul 04, 2020
    Master

    [...] if it's not the case already.

    I don't know what it does, just saying it SHOULD do that if it's not already what this branch does.

    Edited Jul 04, 2020 by LJ Sonic
    > [...] if it's not the case already. I don't know what it does, just saying it SHOULD do that if it's not already what this branch does.
  • toaster @toaster commented Jul 04, 2020
    Master

    Oh, I apologise! Your comment had the vibe of having read the diff more thoroughly than I, not as a statement of what an ideal solution would be doing.

    Oh, I apologise! Your comment had the vibe of having read the diff more thoroughly than I, not as a statement of what an ideal solution would be doing.
  • LJ Sonic @LJSonic commented Jul 04, 2020
    Master

    I am way too lazy to read diffs. :P

    I am *way* too lazy to read diffs. :P
  • James R. @james commented Jul 04, 2020
    Owner

    I really FORGOT

    I really FORGOT
  • toaster @toaster commented Jul 04, 2020
    Master

    And this is why you always need to read diffs thoroughly, even if it's boring.

    And this is why you always need to read diffs thoroughly, even if it's boring.
  • James R. @james commented Jul 04, 2020
    Owner

    Thank you for being the responsible one...

    Thank you for being the responsible one...
  • James R. @james

    added 3 commits

    • 7ec0b6c7 - I meant consvar_number_of_netids to be exclusive :V
    • e741a7b3 - Actually save the cvar name in demos
    • d9005203 - Fuck you macro

    Compare with previous version

    Jul 04, 2020

    added 3 commits

    • 7ec0b6c7 - I meant consvar_number_of_netids to be exclusive :V
    • e741a7b3 - Actually save the cvar name in demos
    • d9005203 - Fuck you macro

    Compare with previous version

    added 3 commits <ul><li>7ec0b6c7 - I meant consvar_number_of_netids to be exclusive :V</li><li>e741a7b3 - Actually save the cvar name in demos</li><li>d9005203 - Fuck you macro</li></ul> [Compare with previous version](https://git.magicalgirl.moe/STJr/SRB2/merge_requests/1010/diffs?diff_id=5838&start_sha=3e52764935cd189a9821c03a2cf63ef6ef2bd5b2)
    Toggle commit list
  • toaster
    @toaster started a discussion on commit d9005203 Jul 04, 2020
    Last updated by toaster Jul 04, 2020
    src/byteptr.h
    150 150
    151 151 #undef DEALIGNED
    152 152
    153 #define WRITESTRINGN(p,s,n) { size_t tmp_i = 0; for (; tmp_i < n && s[tmp_i] != '\0'; tmp_i++) WRITECHAR(p, s[tmp_i]); if (tmp_i < n) WRITECHAR(p, '\0');}
    154 #define WRITESTRING(p,s) { size_t tmp_i = 0; for (; s[tmp_i] != '\0'; tmp_i++) WRITECHAR(p, s[tmp_i]); WRITECHAR(p, '\0');}
    155 #define WRITEMEM(p,s,n) { memcpy(p, s, n); p += n; }
    153 #define WRITESTRINGN(p,s,n) do { size_t tmp_i = 0; for (; tmp_i < n && s[tmp_i] != '\0'; tmp_i++) WRITECHAR(p, s[tmp_i]); if (tmp_i < n) WRITECHAR(p, '\0');} while (0)
    • toaster @toaster commented Jul 04, 2020
      Master

      Before I plusone this branch, what do the changes in this commit do? Does it attempt to run the block once and then exit? Why does this require the do while (0) structure rather than the previously existing curly brace context?

      Before I plusone this branch, what do the changes in this commit do? Does it attempt to run the block once and then exit? Why does this require the `do while (0)` structure rather than the previously existing curly brace context?
    • James R. @james commented Jul 04, 2020
      Owner

      Only one statement can follow if, while etc. You know how you don't need a semicolon after a block? That whole block can basically be considered a single statement.

      So here's an example:

      if (in_demo)
         WRITESTRING(*p, cvar->name);
      else

      So with what the macros were before, that would turn into this:

      if (in_demo)
         { ... }
         ;
      else

      That's basically a block followed by an empty statement. And now the else doesn't follow an if statement. It's just hanging there and that generates a compiler error too.

      if (in_demo)
         do { ... } while (0);
      else

      In C, do while does require a semicolon to end it, so it's a single statement and everything is fine.

      Bonus: You can turn those blocks into single statements by surrounding them with parenthesis too. The READ macros do this.

      Only one statement can follow `if`, `while` etc. You know how you don't need a semicolon after a block? That whole block can basically be considered a single statement. So here's an example: ``` if (in_demo) WRITESTRING(*p, cvar->name); else ``` So with what the macros were before, that would turn into this: ``` if (in_demo) { ... } ; else ``` That's basically a block followed by an empty statement. And now the `else` doesn't follow an `if` statement. It's just hanging there and that generates a compiler error too. ``` if (in_demo) do { ... } while (0); else ``` In C, `do while` does require a semicolon to end it, so it's a single statement and everything is fine. Bonus: You can turn those blocks into single statements by surrounding them with parenthesis too. The `READ` macros do this.
    • toaster @toaster commented Jul 04, 2020
      Master

      bruh

      But it works, so plusoned.

      bruh But it works, so plusoned.
    Please register or sign in to reply
  • James R. @james

    merged

    Jul 11, 2020

    merged

    merged
    Toggle commit list
  • James R. @james

    mentioned in commit 8bcc2662

    Jul 11, 2020

    mentioned in commit 8bcc2662

    mentioned in commit 8bcc2662d1e88e15ad92b3f32599fbcbfeb2e399
    Toggle commit list
  • Write
  • Preview
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment
Assignee
No assignee
Assign to
None
Milestone
None
Assign milestone
Time tracking
2
Labels
Bugs Netcode noodles
Assign labels
  • View project labels
Reference: STJr/SRB2!1010
×

Revert this merge request

This will create a new commit in order to revert the existing changes.

Switch branch
Cancel
A new branch will be created in your fork and a new merge request will be started.
×

Cherry-pick this merge request

Switch branch
Cancel
A new branch will be created in your fork and a new merge request will be started.