Skip to content

fix: implement warning logs on non-owner invoking ServerRpc that requires ownership #627

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 16, 2021
Merged

fix: implement warning logs on non-owner invoking ServerRpc that requires ownership #627

merged 2 commits into from
Mar 16, 2021

Conversation

0xFA11
Copy link
Contributor

@0xFA11 0xFA11 commented Mar 15, 2021

fixes #601

wackoisgod
wackoisgod approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@wackoisgod wackoisgod left a comment

Choose a reason for hiding this comment

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

validated the generated code.

becksebenius-unity
becksebenius-unity approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@becksebenius-unity becksebenius-unity left a comment

Choose a reason for hiding this comment

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

This change looks good, although the fact that you're just now adding Debug_LogWarning_MethodRef makes me concerned that we might be missing other important warning cases

@wackoisgod
Copy link
Contributor

I think that is likely true :P I think until we get better overall testing we are going to be in that state much of the time.

@mattwalsh-unity mattwalsh-unity merged commit 0203fac into release/0.1.0 Mar 16, 2021
@mattwalsh-unity mattwalsh-unity deleted the fix/warn-not-owner branch March 16, 2021 17:12
LukeStampfli
LukeStampfli reviewed Mar 16, 2021
View reviewed changes
com.unity.multiplayer.mlapi/Editor/CodeGen/NetworkBehaviourILPP.cs
switch (methodInfo.Name)
{
case k_Debug_LogWarning:
if (methodInfo.GetParameters().Length == 1) Debug_LogWarning_MethodRef = moduleDefinition.ImportReference(methodInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this use MLAPIs networklog like pretty much everything else in our codebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean doesn't it just goto LogWarning anyway and just add a "[MLAPI]" prefix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but if it is a client logging the warning (which is the case here) then it will also send a network message to the server to display the log there as well. Since this method looks like something we might reuse later for other ILPP stuff we might want to get this right to have logging consistency? Not sure if it is really needed.

This was referenced Mar 16, 2021
Closed
Merged
0xFA11 added a commit that referenced this pull request Mar 23, 2021
@0xFA11 @kvassall-unity
@jeffreyrainy @TwoTenPvP @mattwalsh-unity @lkrell @LukeStampfli @becksebenius-unity @wackoisgod
chore: merge release into develop (#657)
5031f9b 
* fix: implement warning logs on non-owner invoking ServerRpc that requires ownership (#627)

* add todo comments

* fix: implement warning logs on non-owner invoking ServerRpc that requires ownership

* fix: Correcting the profiler names for network vars to be appropriately named. Should address issue #605 (#632)

* test: NetworkTickSystem test (#630)

* test: NetworkTickSystem test

* test: NetworkTickSystem test, removed unneeded code

* fix: Fix object traversal assigning instanceIds to none scene objects (#629)

* fix: Fix object traversal assigning instanceIds to none scene objects

* comment added

Co-authored-by: Matt Walsh <matt.walsh@unity3d.com>

* fix: Correcting the profiler names for named messages vars to be appropriately named. Should address issue #604 (#634)

* fix: prevent OnPerformanceTickData from sending data on null (#614)

If something causes an exception it's possible that the profiler never allocates data. in these cases, there is no point in sending data outside of MLAPI

* fix: use specified transport channel instead of hardcoded reliablerpc channel (#638)

* docs: added release notes to changelog.md (#569)

* Add release notes to changelog

* docs: added release notes to changelog

* Update com.unity.multiplayer.mlapi/CHANGELOG.md

Co-authored-by: Luke Stampfli <43687322+LukeStampfli@users.noreply.github.com>

* update per review for bitserializer and networkserializer

* revised per feedback - API refactor

* update remove bitserializer

* renaming

* Update com.unity.multiplayer.mlapi/CHANGELOG.md

Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

* Update com.unity.multiplayer.mlapi/CHANGELOG.md

* Update com.unity.multiplayer.mlapi/CHANGELOG.md

* added known issues and scene mgmt fix

* revisions per review

* profiler final - no more updates

* format

Co-authored-by: Luke Stampfli <43687322+LukeStampfli@users.noreply.github.com>
Co-authored-by: Matt Walsh <69258106+mattwalsh-unity@users.noreply.github.com>
Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

* docs: Update repository/project docs (#636)

- Mirror package readme.md in root readme.md
- Add repo directory structure description
- Remove contributing and code of conduct from package directory

* docs: Add content for mlapi manual.md (#639)

resolves #553
resolves MTT-534

* fix: Removed per-tick runtime allocations by reusing memory between ticks (#640)

PerformanceDataManager reuses PerformanceTickData by resetting per-frame
Replaces deferred linq query in PerformanceTickData loop.

* fix: fixed network activity occurring outside the core update getting missed in the profiler (#641)

Co-authored-by: Matt Walsh <69258106+mattwalsh-unity@users.noreply.github.com>

* fix: Resolve an issue where 2019.4 would not properly run ILPP with a clean import (#645)

* Add an dummy ILPP to force 2019.4 to proper compilation order

* Turning on this test for 2019.4

* Hack -> Workaround rename

Co-authored-by: Matt Walsh <matt.walsh@unity3d.com>
Co-authored-by: Matt Walsh <69258106+mattwalsh-unity@users.noreply.github.com>

* build: add Unity module dependencies explicitly (#648)

Co-authored-by: kvassall-unity <68920108+kvassall-unity@users.noreply.github.com>
Co-authored-by: Jeffrey Rainy <jeffreyrainy@gmail.com>
Co-authored-by: Albin Corén <2108U9@gmail.com>
Co-authored-by: Matt Walsh <matt.walsh@unity3d.com>
Co-authored-by: Lori Krell <76010626+lkrell@users.noreply.github.com>
Co-authored-by: Luke Stampfli <43687322+LukeStampfli@users.noreply.github.com>
Co-authored-by: Matt Walsh <69258106+mattwalsh-unity@users.noreply.github.com>
Co-authored-by: Jesse Olmer <jesseo@unity3d.com>
Co-authored-by: becksebenius-unity <74328025+becksebenius-unity@users.noreply.github.com>
Co-authored-by: Andrew Spiering <aspiering@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wackoisgod wackoisgod wackoisgod approved these changes

@LukeStampfli LukeStampfli LukeStampfli left review comments

@becksebenius-unity becksebenius-unity becksebenius-unity approved these changes

@TwoTenPvP TwoTenPvP Awaiting requested review from TwoTenPvP

@mattwalsh-unity mattwalsh-unity Awaiting requested review from mattwalsh-unity

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@0xFA11 @wackoisgod @LukeStampfli @becksebenius-unity @mattwalsh-unity