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

Bugfixes & Cleanup #120

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

BrentDaMage
Copy link
Contributor

@BrentDaMage BrentDaMage commented Apr 17, 2024

  • Cleaned up GameMods.hpp & fixed all macro toggles
  • Made world border unpickable/unhighlightable
  • Lowered mob reach range for creative mode from 32 blocks to 7 (as in 0.12.1)
  • Fixed uninitialized variables (bugs could appear depending on the compiler)
    • "PathfinderMob::m_pAttackTarget" (caused a crash)
    • "Entity::field_C4" (caused entities to suffocate always)
  • Fixed Random.cpp for compilation with C++03 by using "std::numeric_limits" instead of "INFINITY"
  • Documented Texture::field_C as m_hasAlpha
  • Deleted unused "GLTexture" struct
  • Removed ModelPart::renderHorrible
  • Added other mob textures to .gitignore

GameMods.hpp Outdated Show resolved Hide resolved
Copy link
Member

@iProgramMC iProgramMC left a comment

Choose a reason for hiding this comment

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

This looks good to me. But I would like an answer to these reviews.

@BrentDaMage
Copy link
Contributor Author

The pull-request build checks are not accurate reflections of the build checks on my bugfix-cleanup branch.

@iProgramMC
Copy link
Member

It probably does. See https://github.com/ReMinecraftPE/mcpe/actions/runs/8761750741/job/24048662614?pr=120#step:4:244

You didn't rename Texture::field_C to m_bHasAlpha fully, apparently.

@BrentDaMage
Copy link
Contributor Author

It probably does. See https://github.com/ReMinecraftPE/mcpe/actions/runs/8761750741/job/24048662614?pr=120#step:4:244

You didn't rename Texture::field_C to m_bHasAlpha fully, apparently.

No? Look at my latest commit on bugfix-cleanup. All of its checks passed.

source/client/app/Minecraft.hpp Show resolved Hide resolved
source/network/Packet.hpp Outdated Show resolved Hide resolved
source/network/packets/StartGamePacket.cpp Outdated Show resolved Hide resolved
source/world/gamemode/CreativeMode.cpp Outdated Show resolved Hide resolved
BrentDaMage added 11 commits May 4, 2024 21:45
* Cleaned up GameMods.hpp & fixed all macro toggles
* Made world border unpickable/unhighlightable
* Lowered mob reach range for creative mode from 32 blocks to 7 (as in 0.12.1)
* Fixed uninitialized variables (bugs could appear depending on the compiler)
    * "PathfinderMob::m_pAttackTarget" (caused a crash)
    * "Entity::field_C4" (caused entities to suffocate always)
* Fixed Random.cpp for compilation with C++03 by using "std::numeric_limits" instead of "INFINITY"
* Documented Texture::field_C as m_hasAlpha
* Deleted unused "GLTexture" struct
* Removed ModelPart::renderHorrible
* Added other mob textures to .gitignore
* It still does not work. I'm not even sure how it broke in the first place.
* Added option & enhancement macro for enabling/disabling the panorama menu background
* Fixed a bug which caused mobs to not catch fire
* Added Flint, Porkchops, and Eggs
* Mobs can now drop loot upon death
* Added Chickens
* Added scaffolding for gamemodes in replication and level data. This can be enabled with 'TEST_GAMEMODE_REPLICATION'.
* Documented a number of variables
* Add EntityType
* Add MobFactory
* Add PacketUtil
* Remove TLong/LongHack remnants
* Remove old minecraftcpp.vcxproj file
* Reverted protocol version back to 2
* Added Cow
* Made tons of stuff const
* Fixed-ish pathfinding
* Replaced global variable with LevelRenderer::areCloudsAvailable()
* Fixed crashing from moving infinite distane
* Minor aesthetic tweaks to DirectConnectScreen
* Cleanup some, but not all, VS project files
* Added missing mob .cpp files to CMakeLists.txt
* Cleaned up Visual Studio project files
* Fixed SDL2 client building for Windows on VS
* Fixed mouse centering on SDL2 clients
platforms/sdl/base/AppPlatform_sdl_base.cpp Outdated Show resolved Hide resolved
platforms/sdl/desktop/AppPlatform_sdl.cpp Outdated Show resolved Hide resolved
source/client/gui/Screen.cpp Outdated Show resolved Hide resolved
* Added ChunkPos
* Added Vec2
* Added Facing::Name enum for cardinal directions
* Added TilePos (this is a must-see!)
* Added ChunkTilePos

Cleaned up input handling
* Added SDL2 controller support
* Added read-only "Use Controller" "option" to visualize current input handler
* Created ControllerBuildInput & ControllerMoveInput
* Moved mouse-specific build-input-handling code from Minecraft class to new MouseBuildInput class
* Refactored BuildActionIntention

Cleaned up VS project files
* Fixed VS include path properties
* Corrected SDL2 client building in VS

Other
* Moved iOS-specific assets to accurate location
* Abstracted EmptyLevelChunk CPP into its own .cpp file
platforms/sdl/base/AppPlatform_sdl_base.cpp Show resolved Hide resolved
platforms/sdl/base/AppPlatform_sdl_base.cpp Outdated Show resolved Hide resolved
platforms/windows/projects/LibPNG/LibPNG.vcxproj Outdated Show resolved Hide resolved
source/CMakeLists.txt Outdated Show resolved Hide resolved
player->startDestroying();
}

bool contDestory = m_pGameMode->continueDestroyBlock(player, m_hitResult.m_tilePos, m_hitResult.m_hitSide);
Copy link
Member

Choose a reason for hiding this comment

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

typo, also unused variable, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean.

vx = dX / dist;
vy = dY / dist;
vz = dZ / dist;
// what about .normalize()?
Copy link
Member

Choose a reason for hiding this comment

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

True, why not add that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See updated code.

@@ -69,7 +69,7 @@ Level::~Level()

//you better HOPE this is freed by Minecraft!
//Really should have used shared pointers and stuff.
if (!pEnt->isLocalPlayer())
//if (!pEnt->isLocalPlayer())
Copy link
Member

Choose a reason for hiding this comment

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

Warning! This hack was added in for a reason -- it's possible that m_pLocalPlayer or m_pMobPersp may have a stale reference after this which would be absolutely terrible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is that possible? From what I can tell, Level is deconstructed in SavingWorldScreen::tick(), then m_pMobPersp and m_pLocalPlayer are set to nullptr.

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the case when I added that line. I'll take a look later and see when this happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this to isPlayer, since the NetworkHandlers are typically in charge of freeing the Player pointers in NetworkPlayer. Originally, the server would crash if a player left who had died, since their entity would be removed by the Level but was still present in the NetworkPlayer.

int minX = Mth::floor(aabb.min.x), minY = Mth::floor(aabb.min.y), minZ = Mth::floor(aabb.min.z),
maxX = Mth::floor(aabb.max.x + 1), maxY = Mth::floor(aabb.max.y + 1), maxZ = Mth::floor(aabb.max.z + 1);
TilePos min(aabb.min),
max(aabb.max + 1);
Copy link
Member

Choose a reason for hiding this comment

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

+TilePos(1) or +TilePos(1,1,1). Mentioned this before but you should NOT have implicit constructors like that.

source/world/level/Region.cpp Outdated Show resolved Hide resolved
: x(_x), y(_y < 0 ? 0 : _y), z(_z) {}

TilePos::TilePos(float _x, float _y, float _z)
: TilePos((int)floorf(_x), (uint8_t)floorf(_y), (int)floorf(_z)) {}
Copy link
Member

Choose a reason for hiding this comment

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

how about using Mth::floor here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because Mojang used floorf here. Is there any good reason for me to be using Mth::floor over floorf?

* Added icon for Win32 client
* Enabled UI button tab navigation for gamepad
* Fixed a couple of incorrect gamepad input mappings
* Mitigated LocalPlayer::isImmobile() bug to gamepad only
* Tweaked ControllerTurnInput to closer match Legacy Console Edition's turn behavior
* Documented various fields
* Made timing more percise for Windows
@iProgramMC iProgramMC dismissed their stale review July 21, 2024 12:33

More commits happened

BrentDaMage and others added 12 commits July 21, 2024 15:18
* Fixed networking bugs
* Added normal lighting support
* Added the Sun and the Moon
* Added entity circle shadows
* Added vignette
* Added manual mob rendering offset to fix floating mobs
* Fixed glColor3f calls for GLES
* Added texture availability check to AppPlatform_iOS
* Fixed building via Xcode
* Set iOS deployment target from 6.0 back to 3.0
* Set default GameMode from Survival back to Creative
@BrentDaMage
Copy link
Contributor Author

BrentDaMage commented Sep 17, 2024

Adds Controller Support #121 for SDL2.

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.

3 participants