Skip to content

use function template#2835

Merged
salix5 merged 15 commits into
Fluorohydride:masterfrom
salix5:patch44
Jul 6, 2025
Merged

use function template#2835
salix5 merged 15 commits into
Fluorohydride:masterfrom
salix5:patch44

Conversation

@salix5

@salix5 salix5 commented Jun 10, 2025

Copy link
Copy Markdown
Collaborator

Problem

Single script:

Debug.SetAIName("AI")
Debug.ReloadFieldBegin(DUEL_ATTACK_FIRST_TURN+DUEL_SIMPLE_AI)
Debug.SetPlayerInfo(0,800,0,0)
Debug.SetPlayerInfo(1,8000,0,0)

Debug.AddCard(39910367,0,0,LOCATION_SZONE,5,POS_FACEUP_ATTACK)
Debug.AddCard(65342096,0,0,LOCATION_HAND,0,POS_FACEDOWN_DEFENSE)
Debug.ReloadFieldEnd()

39910367
魔法都市エンディミオン
①:このカードがフィールドゾーンに存在する限り、自分または相手が魔法カードを発動する度に、このカードに魔力カウンターを1つ置く。

Change line 47 of c39910367.lua

e:GetHandler():AddCounter(0x1,65530)

(put 65530 counters)

圖片

The maximum number of counters is 65535, so the players can put 65530 counters.

Solution

Now we do not use int8_t and int16_t.
For types smaller than int, we only use uint8_t and uint16_t.

@mercury233
@purerosefallen
@Wind2009-Louse
@fallenstardust

@salix5 salix5 requested a review from Copilot June 10, 2025 16:16

This comment was marked as outdated.

@mercury233

Copy link
Copy Markdown
Collaborator

Sorry, I can't get why this change should be done

@salix5

salix5 commented Jun 11, 2025

Copy link
Copy Markdown
Collaborator Author

ocgcore一直以來都是用uint8, uint16
如果接收時強制轉成int16就可能變成負數
現在只是修正成uint16

@purerosefallen

Copy link
Copy Markdown
Collaborator

plz keep both ReadUInt* and ReadInt*

@purerosefallen

Copy link
Copy Markdown
Collaborator

In fact

uint16_t x = BufferIO::ReadInt16(p);

it's still uint16_t, without problems

@salix5

salix5 commented Jun 12, 2025

Copy link
Copy Markdown
Collaborator Author

plz keep both ReadUInt* and ReadInt*

done

In fact

uint16_t x = BufferIO::ReadInt16(p);

it's still uint16_t, without problems

If you want to read uint16_t, maybe you should read uint16_t not int16_t.

@salix5 salix5 changed the title remove ReadInt8, ReadInt16 use ReadUInt8, ReadUInt16 Jun 12, 2025
@purerosefallen

Copy link
Copy Markdown
Collaborator

plus could we add ReadUInt32?

@salix5

salix5 commented Jun 12, 2025

Copy link
Copy Markdown
Collaborator Author

Maybe you can use buffer_read directly:

buffer_read<uint32_t>(p);

In C++, the project should not declare a read function for each integer type in the first place.
But I don't want to modify 700 lines and spend another 700 hours arguing why it is necessary.
So I only modify the definition in bufferio.h.

Other types can be read/write by buffer_read, buffer_write.

@purerosefallen

Copy link
Copy Markdown
Collaborator

Maybe you can use buffer_read directly:

buffer_read<uint32_t>(p);

In C++, the project should not declare a read function for each integer type in the first place. But I don't want to modify 700 lines and spend another 700 hours arguing why it is necessary. So I only modify the definition in bufferio.h.

Other types can be read/write by buffer_read, buffer_write.

Just add it, and it may not be needed to use, making it tidy to have every ReadInt and ReadUInt

@salix5

salix5 commented Jun 12, 2025

Copy link
Copy Markdown
Collaborator Author

We should not keep every ReadInt*, ReadInt* in the first place.
There are 6 combinations of ReadInt/ReadUint + 8/16/32.
If ygopro uses more types like int64/uint64/char8_t/char16_t in the future, the combinations will increase even more.

Using 6~10 declarations for 1 function template is not tidy.
It is insane.

I know it is insane, but I don't want to modify 700 lines and spend another 700 hours arguing why it is much better.
That is why I keep these insane declarations.
Please do not add more insane declarations like that.

@salix5 salix5 requested a review from Copilot June 12, 2025 19:50

This comment was marked as outdated.

@salix5 salix5 changed the title use ReadUInt8, ReadUInt16 use buffer_read template Jun 13, 2025
Comment thread gframe/duelclient.cpp Outdated
@mercury233

Copy link
Copy Markdown
Collaborator

I don't think there's anything wrong with utility functions. Writing them is a one-time effort, and they are simpler to use later on compared to buffer_read<>.

@salix5

salix5 commented Jun 14, 2025

Copy link
Copy Markdown
Collaborator Author

function template的其中一個用途是
避免替每個不同的變數型態都宣告一個新的函數
每個型態都宣告一個Read*, Write*函數不只大幅增加不必要的宣告
輸入時仍然要從這些名稱相近的列表中選擇正確的項目

與此相比
function template可以在輸入時直接指定任意型態
型態還可以讓編譯器自動推斷
也就是<>裡面也可以不用指定

我修改時是直接取代因此都會指定型態
如果有些地方有需要大量讀取/寫入相同類型的變數
要用自動推斷亦無不可
我想這應該會比宣告6~10個功能相同的函數簡單很多

@salix5 salix5 requested a review from Copilot June 14, 2025 03:15

This comment was marked as outdated.

@mercury233

Copy link
Copy Markdown
Collaborator

how about we use BufferIO::Read<> instead of relying on the ocgcore buffer.h

@salix5

salix5 commented Jun 14, 2025

Copy link
Copy Markdown
Collaborator Author

Do you mean

template<typename T>
static T Read(unsigned char*& p) {
	T ret{};
	std::memcpy(&ret, p, sizeof(T));
	p += sizeof(T);
	return ret;
};

@mercury233

Copy link
Copy Markdown
Collaborator

Do you mean

template<typename T>
static T Read(unsigned char*& p) {
	T ret{};
	std::memcpy(&ret, p, sizeof(T));
	p += sizeof(T);
	return ret;
};

yes

This comment was marked as outdated.

@salix5 salix5 changed the title use buffer_read template use function template Jun 15, 2025
@purerosefallen

Copy link
Copy Markdown
Collaborator

too many things to do this update. I think we could do it after the Jul update (1362)

@salix5 salix5 added the future label Jun 21, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces the legacy BufferIO::ReadIntX/WriteIntX methods and buffer_read/buffer_write calls with new templated BufferIO::Read<T> and BufferIO::Write<T> to unify integer I/O and deprecate smaller signed types.

  • Introduces template<typename T> Read/Write in bufferio.h and marks the old methods as deprecated.
  • Migrates all read/write calls in the gframe modules to the new templated interfaces.
  • Removes the ocgcore/buffer.h dependency.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

File Description
gframe/bufferio.h Added templated Read/Write, deprecated old APIs.
gframe/tag_duel.cpp Replaced all WriteIntX/ReadUIntX with templates.
gframe/netserver.cpp Updated packet serialization with templated I/O.
Comments suppressed due to low confidence (2)

gframe/bufferio.h:21

  • [nitpick] The deprecated ReadIntX/WriteIntX methods are still present alongside the new templated interfaces. Once migration is complete, remove these deprecated methods to reduce API surface and avoid confusion.
	}

gframe/tag_duel.cpp:299

  • [nitpick] This pattern of casting and templated write is repeated many times. Consider introducing a helper macro or inline function (e.g., WriteCount(pbuf, pdeck[i].main.size())) to reduce boilerplate and the risk of inconsistent usage.
	BufferIO::Write<uint16_t>(pbuf, (uint16_t)pdeck[0].main.size());

Comment thread gframe/bufferio.h
@salix5 salix5 merged commit 42a427a into Fluorohydride:master Jul 6, 2025
8 checks passed
@salix5 salix5 deleted the patch44 branch July 6, 2025 05:18
@salix5 salix5 removed the future label Jul 16, 2025
@purerosefallen

Copy link
Copy Markdown
Collaborator

question, for eco management:

How to make replacement?

@salix5

@salix5

salix5 commented Jul 23, 2025

Copy link
Copy Markdown
Collaborator Author

BufferIO::ReadInt32
BufferIO::Read<int32_t>

BufferIO::ReadInt16
not recommended
prefer:
BufferIO::Read<uint16_t>

BufferIO::ReadInt8
not recommended
prefer:
BufferIO::Read<uint8_t>

BufferIO::ReadUInt8
BufferIO::Read<uint8_t>

BufferIO::WriteInt32
BufferIO::Write<int32_t>

BufferIO::WriteInt16
not recommended
prefer:
BufferIO::Write<uint16_t>

BufferIO::WriteInt8
not recommended
prefer:
BufferIO::Write<uint8_t>

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.

4 participants