tatsumi - begin some refactoring #13418

mamehaze · 2025-02-25T15:24:09Z

split rotating sprites into a device
split into 3 drivers (apache3, roundup5, cyclwarr) as apart from sprites they have little actually in common
a few small cleanups

…g fake colours on the end of old palette

mamehaze · 2025-02-25T19:59:27Z

note, a lot of the code here still isn't very good, this is mostly a restructuring to make future work easier / more manageable, it isn't a code clean-up pass as such so you're going to see a lot of ugly code moved around.

I do plan on looking at things like the sprite rotation again at some point, I did have some old progress but appear to have lost it, as I tried to take on too much at once without committing anything, rather than submit a smaller part as I'm doing here.

cuavas

Please use anonymous namespaces in self-contained driver source files to help keep linking time down, avoid symbol conflicts, etc.

cuavas · 2025-02-28T13:28:02Z

src/mame/tatsumi/cyclwarr.cpp

+    - (fixed) Cycle Warriors: transparent road layer on sidelines, wrong mask_data?
+    - (fixed) Missing BG layer (Round Up 5) - banked VRAM data from somewhere!?
+    - (fixed?) Cycle Warriors: test mode text does not appear as it needs a -256 Y
+      scroll offset from somewhere.
+    - (fixed) Cycle Warriors: sometimes it draws garbage on character select or even hangs
+      depending on where player coins up, most likely caused by miscommunication with sub CPU?
+    - (fixed) Cycle Warriors: ranking screen is completely wrong;
+    - (fixed) Cycle Warriors: ugly orange color on character select and briefing screens, layer disable?


There’s no point keeping the stuff that’s done in the TODO list…

do keep the (fixed?) comment though, you never know when that'll turn out to be a massive oversight.

iirc text layer should come from 6845 so it's something that needs doing anyway, the note is safe to remove imo.

Actually nm: this is tatsumi/cyclwarr.cpp not roundup5.cpp, don't remember what's that for then.

once again, this is just from the existing files, the goal here was to preserve what was there, if a decision is made to not want that information it's really something entirely separate from this PR which you just seem to be piling additional requests onto outside the scope of what's being done.

cuavas · 2025-02-28T13:28:46Z

src/mame/tatsumi/cyclwarr.cpp

+	tileinfo.set(0,
+						 tileno,
+						 color,
+						opaque ? TILE_FORCE_LAYER0 : 0);


Indentation is all over the place here.

again this hasn't actually changed from MAME, please keep the scope of the feedback to things I've actually changed.

cuavas · 2025-02-28T13:29:22Z

src/mame/tatsumi/cyclwarr.cpp

+	tileinfo.set(0,
+						 tileno,
+						 color | m_road_color_bank,
+						 opaque ? TILE_FORCE_LAYER0 : 0);


again this hasn't actually changed from MAME, please keep the scope of the feedback to things I've actually changed.

cuavas · 2025-02-28T13:32:31Z

src/mame/tatsumi/roundup5.cpp

+	DECLARE_VIDEO_START(roundup5);
+	uint32_t screen_update_roundup5(screen_device &screen, bitmap_rgb32 &bitmap, const rectangle &cliprect);


You should just override video_start() for this derived class rather than using the legacy VIDEO_START stuff.

This hasn't changed from what's already in MAME, again not really eligible for PR review here. I may change it anyway, but again please concentrate on what I have changed, picking up on things I haven't changed at all is not helpful.

src/mame/tatsumi/roundup5.cpp

cuavas · 2025-02-28T13:43:32Z

src/mame/tatsumi/tatsumi_rotating_sprites.h

+
+	DECLARE_GFXDECODE_MEMBER(gfxinfo);


Does this need to be public?

cuavas · 2025-02-28T13:44:29Z

src/mame/tatsumi/tatsumi_rotating_sprites.h

+	std::unique_ptr<uint8_t[]> m_shadow_pen_array;
+
+	bitmap_rgb32 m_temp_bitmap;
+
+	int m_sprite_palette_base;
+
+	void mycopyrozbitmap_core(bitmap_ind8 &bitmap, const bitmap_rgb32 &srcbitmap,
+			int dstx, int dsty, int srcwidth, int srcheight, int incxx, int incxy, int incyx, int incyy,
+			const rectangle &clip, int transparent_color);
+	void mycopyrozbitmap_core(bitmap_rgb32 &bitmap, const bitmap_rgb32 &srcbitmap,
+			int dstx, int dsty, int srcwidth, int srcheight, int incxx, int incxy, int incyx, int incyy,
+			const rectangle &clip, int transparent_color);
+
+	template<class BitmapClass> void draw_sprites_main(BitmapClass &bitmap, const rectangle &cliprect, int write_priority_only, int rambank);
+	template<class BitmapClass> inline void roundupt_drawgfxzoomrotate( BitmapClass &dest_bmp, const rectangle &clip,
+		gfx_element *gfx, uint32_t code,uint32_t color,int flipx,int flipy,uint32_t ssx,uint32_t ssy,
+		int scalex, int scaley, int rotate, int write_priority_only );
+
+	required_shared_ptr<uint16_t> m_spriteram;
+	required_region_ptr<uint8_t> m_sprites_l_rom;
+	required_region_ptr<uint8_t> m_sprites_h_rom;


Please keep the data members together and the member functions together.

cuavas · 2025-02-28T13:49:53Z

src/mame/tatsumi/tatsumi_rotating_sprites.cpp

+void tatsumi_rotating_sprites_device::device_add_mconfig(machine_config &config)
+{
+	PALETTE(config, m_fakepalette).set_format(palette_device::xRGB_555, 4096); // 4096 arranged as series of CLUTs
+}
+
+void tatsumi_rotating_sprites_bigpal_device::device_add_mconfig(machine_config &config)
+{
+	PALETTE(config, m_fakepalette).set_format(palette_device::xRGB_555, 8192); // 8192 arranged as series of CLUTs
+}


You don’t need to override this in the derived class, you can just use m_rom_clut_size * 2 for the number of entries.

cuavas · 2025-02-28T13:51:40Z

src/mame/tatsumi/tatsumi_rotating_sprites.h

+	virtual void device_start() override ATTR_COLD;
+	virtual void device_reset() override ATTR_COLD;
+
+	void common_init();


If this is only used at start time, it should be marked cold, too.

mamehaze · 2025-02-28T13:56:22Z

Most of your comments here are about the existing code, that will be cleaned up later, not in this PR, please stop doing that, it makes incremental progress impossible. The code is no worse than it already is in mame. Anything specific to what I've done ill fix.

…

On Fri, 28 Feb 2025, 13:52 Vas Crabb, ***@***.***> wrote: ***@***.**** requested changes on this pull request. Please use anonymous namespaces in self-contained driver source files to help keep linking time down, avoid symbol conflicts, etc. ------------------------------ In src/mame/tatsumi/cyclwarr.cpp <#13418 (comment)>: > + - (fixed) Cycle Warriors: transparent road layer on sidelines, wrong mask_data? + - (fixed) Missing BG layer (Round Up 5) - banked VRAM data from somewhere!? + - (fixed?) Cycle Warriors: test mode text does not appear as it needs a -256 Y + scroll offset from somewhere. + - (fixed) Cycle Warriors: sometimes it draws garbage on character select or even hangs + depending on where player coins up, most likely caused by miscommunication with sub CPU? + - (fixed) Cycle Warriors: ranking screen is completely wrong; + - (fixed) Cycle Warriors: ugly orange color on character select and briefing screens, layer disable? There’s no point keeping the stuff that’s done in the TODO list… ------------------------------ In src/mame/tatsumi/cyclwarr.cpp <#13418 (comment)>: > + tileinfo.set(0, + tileno, + color, + opaque ? TILE_FORCE_LAYER0 : 0); Indentation is all over the place here. ------------------------------ In src/mame/tatsumi/cyclwarr.cpp <#13418 (comment)>: > + tileinfo.set(0, + tileno, + color | m_road_color_bank, + opaque ? TILE_FORCE_LAYER0 : 0); Same here. ------------------------------ In src/mame/tatsumi/roundup5.cpp <#13418 (comment)>: > + DECLARE_VIDEO_START(roundup5); + uint32_t screen_update_roundup5(screen_device &screen, bitmap_rgb32 &bitmap, const rectangle &cliprect); You should just override video_start() for this derived class rather than using the legacy VIDEO_START stuff. ------------------------------ In src/mame/tatsumi/roundup5.cpp <#13418 (comment)>: > +void roundup5_state::roundup5_control_w(offs_t offset, uint16_t data, uint16_t mem_mask) +{ + COMBINE_DATA(&m_control_word); + + if (m_control_word & 0x10) + m_subcpu->set_input_line(INPUT_LINE_HALT, ASSERT_LINE); + else + m_subcpu->set_input_line(INPUT_LINE_HALT, CLEAR_LINE); + + if (m_control_word & 0x4) + m_audiocpu->set_input_line(INPUT_LINE_HALT, ASSERT_LINE); + else + m_audiocpu->set_input_line(INPUT_LINE_HALT, CLEAR_LINE); + +// if (offset == 1 && (tatsumi_control_w & 0xfeff) != (last_bank & 0xfeff)) +// logerror("%s: Changed bank to %04x (%d)\n", m_maincpu->pc(), tatsumi_control_w,offset); + +//todo - watchdog + + /* Bank: + + 0x0017 : OBJ banks + 0x0018 : 68000 RAM mask 0x0380 used to save bits when writing + 0x0c10 : 68000 ROM + + 0x0040 : Z80 rom (lower half) mapped to 0x10000 + 0x0060 : Z80 rom (upper half) mapped to 0x10000 + + 0x0080 : enabled when showing map screen after a play + (switches video priority between text layer and sprites) + + 0x0100 : watchdog. + + 0x0c00 : vram bank (bits 0x7000 also set when writing vram) + + 0x8000 : set whenever writing to palette ram? + + Changed bank to 0060 (0) + */ + + if ((m_control_word & 0x8) == 0 && !(m_last_control & 0x8)) + m_subcpu->set_input_line(INPUT_LINE_IRQ4, ASSERT_LINE); +// if (tatsumi_control_w&0x200) +// cpu_set_reset_line(1, CLEAR_LINE); +// else +// cpu_set_reset_line(1, ASSERT_LINE); + +// if ((tatsumi_control_w&0x200) && (last_bank&0x200)==0) +// logerror("68k irq\n"); +// if ((tatsumi_control_w&0x200)==0 && (last_bank&0x200)==0x200) +// logerror("68k reset\n"); + + if (m_control_word == 0x3a00) + { +// cpu_set_reset_line(1, CLEAR_LINE); +// logerror("68k on\n"); + } + + m_last_control = m_control_word; +} This is pretty gross. m_last_control will always equal m_control_word outside this function, so it’s pointless making both of them members. It would be more sensible to make the new control word value a local variable, and update m_control_word at the end. Also, it’s going to be going through the scheduler every time the halt lines for m_audiocpu and m_subcpu *don’t* change because it isn’t checking that they’ve actually changed. ------------------------------ In src/mame/tatsumi/tatsumi_rotating_sprites.h <#13418 (comment)>: > + + DECLARE_GFXDECODE_MEMBER(gfxinfo); Does this need to be public? ------------------------------ In src/mame/tatsumi/tatsumi_rotating_sprites.h <#13418 (comment)>: > + std::unique_ptr<uint8_t[]> m_shadow_pen_array; + + bitmap_rgb32 m_temp_bitmap; + + int m_sprite_palette_base; + + void mycopyrozbitmap_core(bitmap_ind8 &bitmap, const bitmap_rgb32 &srcbitmap, + int dstx, int dsty, int srcwidth, int srcheight, int incxx, int incxy, int incyx, int incyy, + const rectangle &clip, int transparent_color); + void mycopyrozbitmap_core(bitmap_rgb32 &bitmap, const bitmap_rgb32 &srcbitmap, + int dstx, int dsty, int srcwidth, int srcheight, int incxx, int incxy, int incyx, int incyy, + const rectangle &clip, int transparent_color); + + template<class BitmapClass> void draw_sprites_main(BitmapClass &bitmap, const rectangle &cliprect, int write_priority_only, int rambank); + template<class BitmapClass> inline void roundupt_drawgfxzoomrotate( BitmapClass &dest_bmp, const rectangle &clip, + gfx_element *gfx, uint32_t code,uint32_t color,int flipx,int flipy,uint32_t ssx,uint32_t ssy, + int scalex, int scaley, int rotate, int write_priority_only ); + + required_shared_ptr<uint16_t> m_spriteram; + required_region_ptr<uint8_t> m_sprites_l_rom; + required_region_ptr<uint8_t> m_sprites_h_rom; Please keep the data members together and the member functions together. ------------------------------ In src/mame/tatsumi/tatsumi_rotating_sprites.cpp <#13418 (comment)>: > +void tatsumi_rotating_sprites_device::device_add_mconfig(machine_config &config) +{ + PALETTE(config, m_fakepalette).set_format(palette_device::xRGB_555, 4096); // 4096 arranged as series of CLUTs +} + +void tatsumi_rotating_sprites_bigpal_device::device_add_mconfig(machine_config &config) +{ + PALETTE(config, m_fakepalette).set_format(palette_device::xRGB_555, 8192); // 8192 arranged as series of CLUTs +} You don’t need to override this in the derived class, you can just use m_rom_clut_size * 2 for the number of entries. ------------------------------ In src/mame/tatsumi/tatsumi_rotating_sprites.h <#13418 (comment)>: > + void draw_sprites(bitmap_rgb32& bitmap, const rectangle& cliprect, int write_priority_only, int rambank); + void draw_sprites(bitmap_ind8& bitmap, const rectangle& cliprect, int write_priority_only, int rambank); + + void update_cluts(); + + DECLARE_GFXDECODE_MEMBER(gfxinfo); + +protected: + tatsumi_rotating_sprites_device(const machine_config &mconfig, device_type type, const char *tag, device_t *owner, uint32_t clock); + + virtual void device_add_mconfig(machine_config &config) override ATTR_COLD; + + virtual void device_start() override ATTR_COLD; + virtual void device_reset() override ATTR_COLD; + + void common_init(); If this is only used at start time, it should be marked cold, too. — Reply to this email directly, view it on GitHub <#13418 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BBR6GZNQ7VT47IYSY2ETZLD2SBS3LAVCNFSM6AAAAABX25XDTCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNJQG42DQOBXGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

David Haywood added 14 commits February 25, 2025 15:22

tatsumi - begin some refactoring

535d9ab

use separate palette for sprites (for now at least) rather than havin…

0b8f16a

…g fake colours on the end of old palette

just calculate this when needed with an offset

19902c2

simplify update cluts

6f96cb7

more prep for sprite device

6ff92f7

make apache3 palette size match ramsize

4c91988

start moving things around (some breakage for now)

eced801

fix regression

5a1c346

further refactoring

b6daf87

move gfxdecodes into device

054aa1d

better code

4f6295e

begin splitting driver file up

39dd4ad

more splitting

d615ae3

shuffle some bits around

afe7838

mamehaze marked this pull request as ready for review February 25, 2025 19:58

cuavas requested changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tatsumi - begin some refactoring #13418

tatsumi - begin some refactoring #13418

mamehaze commented Feb 25, 2025 •

edited

Loading

mamehaze commented Feb 25, 2025 •

edited

Loading

cuavas left a comment

cuavas Feb 28, 2025

modusc896d352 Feb 28, 2025

angelosa Feb 28, 2025

angelosa Feb 28, 2025

mamehaze Mar 1, 2025

cuavas Feb 28, 2025

mamehaze Mar 1, 2025

cuavas Feb 28, 2025

mamehaze Mar 1, 2025

cuavas Feb 28, 2025

mamehaze Mar 1, 2025

cuavas Feb 28, 2025

cuavas Feb 28, 2025

cuavas Feb 28, 2025

cuavas Feb 28, 2025

mamehaze commented Feb 28, 2025 via email

		DECLARE_VIDEO_START(roundup5);
		uint32_t screen_update_roundup5(screen_device &screen, bitmap_rgb32 &bitmap, const rectangle &cliprect);

tatsumi - begin some refactoring #13418

Are you sure you want to change the base?

tatsumi - begin some refactoring #13418

Conversation

mamehaze commented Feb 25, 2025 • edited Loading

mamehaze commented Feb 25, 2025 • edited Loading

cuavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamehaze commented Feb 28, 2025 via email

mamehaze commented Feb 25, 2025 •

edited

Loading

mamehaze commented Feb 25, 2025 •

edited

Loading